ci: add clippy results to GitHub code scans

[abrown]

In the past, we've overlooked clippy warnings that get lost in the CI build logs. This change would collect all of those warnings, put them in SARIF form, and list them in GitHub's code scanning view. I recently added this to ittapi and it looks like this: Code Scanning. This means warnings and errors will show up on the security tab as a notification; the UI allows one to dismiss the warnings. There might be some integration with PRs but I haven't experimented with that.

I configured this to also run periodically (every Tuesday night); we can remove that if we only want commits to main, e.g. If we do adopt this, we should think about what to do with the clippy job in main.yml--does it stay or go?

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[fitzgen]

Because we already gate PRs on clippy, I don't think this would ever catch anything? Am I missing something? I guess for new versions of clippy with new lints? But we configure crates to inherit the workspace lints, which allows all clippy rules by default, and then we only deny select ones.

In general, I'd rather catch these things at the time they're authored (ie in the PR) than after the fact on the next Tuesday.

But I feel like I might be missing something here...

[abrown]

This code scan is configured to run on every PR and on Tuesdays. The results of code scans during a PR should be available in the PR itself (docs... IIUC). The results of code scans otherwise (on main and on Tuesdays) should show up in GitHub's Code Scanning UI.

This PR is not about adding any new scan functionality, it's about integrating better with GitHub:

• warnings should show up in the UI instead of being lost in build logs; this means that we could enable more warnings if we wanted to
• errors should show up as PR comments; this means we don't have to dig through build logs to see what failed CI
• if any issues somehow get through the review process, they show up in the "Security" tab; this improves visibility in the UI for any warnings

[abrown]

One other comment: my recollection is that we limit our clippy lints to allow or deny only because of the lack of visibility into warnings. My take is that, beyond just the "use the nice tools better" argument, if we wanted to be more picky and warn (but not deny) in more cases, this would be the way to do that.

[alexcrichton]

I share similar confusion to @fitzgen about the state as-is here in the sense that we're "guaranteed clippy clean" on all PRs because we -Dwarnings in CI and also run clippy on CI. Or at least for the "run continuously and have a dashboard" aspect I don't think this'll be too useful without other changes.

Now better surfacing errors in CI to PRs, that sounds great! Do you have an example of what that might look like? Ideally we could surface things like normal compile errors too, but I'm not sure how it works. Or could you perhaps enable a new clippy lint on this PR and we can see what the UI looks like? (assuming enabling a new clippy lint will have lots of errors)

For exploring this a bit, it looks like the links you have (e.g. Code Scanning) are unfortunately only available to maintainers. (it's a 404 for me). I think that means that the dashboard access wouldn't be accessible to contributors-at-large and instead just maintainers?

I can also expand a bit more on the Clippy bits we have in that you're right that we allow-by-default all clippy lints and then selectively enable them. One of the purposes for this is that it's not always the easiest to get PRs from contributors that run cargo clippy and then make thousands of lines of changes to appease clippy. Clippy's defaults are, at least in my opinion, not always the best suggestions and are not universally applicable. This means that the allow-by-default policy isn't just because we want clean CI, it's also that we don't want to get accidentally overburdened by contributors sending clippy fixes.

So overall I'd love to get a better UI for surfacing CI errors into PRs, but I don't think that some of the other benefits you've outlined will be as applicable to us?

[alexcrichton]

On the clippy front I can also expand a bit in that it's fine to enable more clippy lints. The only caveat is that it'll want to have some motivation for why it's applicable to Wasmtime or general agreement that Clippy's suggestions are "basically alwasy right" and not accidentally reducing the quality fo the code. For example #9025 enabled a clippy lint for the whole workspace and #9209 enabled a lint for just one tree of modules.

This reminds me that we should document this policy...

[fitzgen]

+1 to what Alex said. If we can make clippy errors more visible and more easily parseable in CI failures, that's great. But if this makes them only visible to maintainers, that's not really gonna work. And the cron-job aspect is something we shouldn't need.

[bjorn3]

Github actions supports problem matches to parse compiler errors and show then inline in the Files tab of a PR.

[abrown]

Any suggestions on a clippy lint to enable to check the GitHub PR interaction?

[alexcrichton]

I randomly selected clippy::question_mark locally and got a few warnings perhaps? I'm not sure how the integration here will work with warnings-on-files-the-PR-didn't-touch though.

[abrown]

I think what may be going on here is that, since warnings are upgraded to errors, the clippy task will just fail.