Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't suggest similar method when unstable #109212

Merged
merged 1 commit into from
Mar 19, 2023

Conversation

Ezrashaw
Copy link
Contributor

Fixes #109177

Don't display typo suggestions for unstable things, unless the feature flag is enabled.

AFAIK, there are two places this occurs:

  • rustc_resolve: before type checking, effectively just FnCtxt::Free.
  • rustc_hir_typck: during type checking, for FnCtxt::Assoc(..)s.

The linked issue is about the latter, obviously the issue is applicable to both.

r? @estebank

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 16, 2023
@Ezrashaw
Copy link
Contributor Author

Big problems with the rustc_resolve half, not entirely sure why. I'll have to deal with this tomorrow.

@rust-log-analyzer

This comment has been minimized.

// note that `DUMMY_SP` is ok here because it is only used for
// suggestions and macro stuff which isn't applicable here.
!matches!(
self.tcx.eval_stability(candidate.item.def_id, None, DUMMY_SP, None),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this call might be introducing a cycle, which manifests as an .unwrap() ICE. Can you check if calling tcx.lookup_stability(def_id) also hits it?

Copy link
Contributor Author

@Ezrashaw Ezrashaw Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I was vaguely thinking. tcx.lookup_stability isn't what we want either, it calls eval_stability internally and emits an error if the DefId is unstable (definitely not what we want). (EDIT: I'm talking about check_stability, lookup_stabilty is the low-level one 🤦) I'm not sure how to fix the cycle.

@Ezrashaw
Copy link
Contributor Author

Ezrashaw commented Mar 17, 2023

@estebank The offending code is in compiler/rustc_middle/src/middle/stability.rs:417:

let is_staged_api = self.lookup_stability(def_id.krate.as_def_id()).is_some();
if !is_staged_api {
    return EvalResult::Allow;
}

The lookup_stability query requires it's DefId arg to be a LocalDefId, however it caches stuff to disk and I think that's where we are getting stuck; a query is not cached for a non-local crate and then it's trying to create that information and recursing. (I'm newish to the compiler and thus have a very loose grasp on the query system)

I have no idea on how to fix this.

@estebank
Copy link
Contributor

Let's land the typeck part of this for now and keep investigating the resolve version. It's likely we'll have to add some tracking to Resolve for local stabilities and check that. In the meantime, only unstable methods coming from the crate the user wrote will be suggested, which means the author would have enough context to deal with the situation.

@Ezrashaw Ezrashaw force-pushed the no-similar-sugg-for-unstable branch from 0544dc1 to 96603bd Compare March 17, 2023 23:27
@Ezrashaw
Copy link
Contributor Author

@estebank Sure, I'm happy to land just the typeck part, I'll be looking into the resolve part a bit (not sure the fix is easy).

@rust-log-analyzer

This comment has been minimized.

@Ezrashaw Ezrashaw force-pushed the no-similar-sugg-for-unstable branch from 96603bd to 0dc36fc Compare March 18, 2023 03:19
@estebank
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 19, 2023

📌 Commit 0dc36fc has been approved by estebank

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 19, 2023

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 19, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2023
Rollup of 10 pull requests

Successful merges:

 - rust-lang#104100 (Allow using `Range` as an `Iterator` in const contexts. )
 - rust-lang#105793 (Add note for mismatched types because of circular dependencies)
 - rust-lang#108798 (move default backtrace setting to sys)
 - rust-lang#108829 (Use Edition 2021 :pat in matches macro)
 - rust-lang#108973 (Beautify pin! docs)
 - rust-lang#109003 (Add `useless_anonymous_reexport` lint)
 - rust-lang#109022 (read_buf_exact: on error, all read bytes are appended to the buffer)
 - rust-lang#109212 (fix: don't suggest similar method when unstable)
 - rust-lang#109243 (The name of NativeLib will be presented)
 - rust-lang#109324 (Implement FixedSizeEncoding for UnusedGenericParams.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 879d6f2 into rust-lang:master Mar 19, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 19, 2023
@Ezrashaw Ezrashaw deleted the no-similar-sugg-for-unstable branch March 20, 2023 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustc suggests unstable library method on stable
5 participants