Use a dummy TypeckResults instead of Option<TypeckResults> when linting

[Jarcho]

View all comments

Based on #157928

This brings the typeck results when linting back to what it was before #73743. The first commit adds a dummy ownerless typeck results and uses that as the initial value for the lint pass. The second commit just removes the now useless typeck_results method.

The dummy table is added to the global context more for convenience. It can't be a static since the lifetime argument is invariant. It could be created just for the linting, but that would need to add an extra lifetime parameter to LateContext and a lot of places downstream of that which would be a huge pain.

[rustbot]

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

[rustbot]

r? @folkertdev

rustbot has assigned @folkertdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 20 candidates

[folkertdev]

r? types

[Jarcho]

The line the error is complaining about doesn't even exist anywhere. What is the miri test even building?

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[rust-bors]

☔ The latest upstream changes (presumably #161396) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

[BoxyUwU]

r? types

[jackh726]

Hmm. I see the appeal - but I don't like losing the type safety of having this be optional. I would sooner like to see us refactor TypeckResults into something that's "always available" and what is only "sometimes available" - or to move this to a place where we don't incidentally want it around for linting.

[oli-obk]

TypeckResults can't really be split up meaningfully (or I'm misunderstanding what you want to do). There is nothing that is always available (or well, this PR already splits it into "always available: empty tables", "sometimes available: owner def id".

If we want to have more type safety but also fewer unwraps, we can split the lint contexts and traits into "lint contexts in body" and "lint context without body", but not sure that is worth it.

There is also another option where everything outside the body has an owner id of the crate root or sth. This will incidentally work for all the checks we have right now, but it may be too subtle. Though I struggle to see where it would ever cause us problems

[Jarcho]

I'm unclear what type safety you get from the option given that it's just unwrapped everywhere. Modelling the option as a degenerate value which just does the right thing (crash when accessing) when performing the check we already want (that we have the correct table) avoids unwrapping it while still keeping the same level of correctness we currently have. Unwrapping will crash earlier, but only marginally.

There is also another option where everything outside the body has an owner id of the crate root or sth. This will incidentally work for all the checks we have right now, but it may be too subtle. Though I struggle to see where it would ever cause us problems

Using an optional owner will outright crash every query with a mismatching owner. Given that it's always nonsense to query the non-body results this seems like the better outcome.

[jackh726]

If we want to have more type safety but also fewer unwraps, we can split the lint contexts and traits into "lint contexts in body" and "lint context without body", but not sure that is worth it.

This is where I was going, yeah.

Unwrapping will crash earlier, but only marginally.

Perhaps this is true, but perhaps not. I don't really think it's true that you can do nothing with a dummy typeck context with this PR (or, that there isn't a chance someone adds that in the future) - where you definitely cannot today. My worry is that somebody goes to use typeck results somewhere were they don't actually have typeck results and get "incorrect" data.