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

diagnostic: properly deal with hygienic names on unresolved fields and imports #116575

Closed
wants to merge 2 commits into from

Conversation

fmease
Copy link
Member

@fmease fmease commented Oct 9, 2023

Follow-up to #116429:

  • Previously I didn't adjust the usage-site span of struct fields to the expansion of the def-site before comparing syntax contexts which lead to us not suggesting names from “parent” syntax contexts (see the test case involving the top-level struct Case and the macro environment)
  • I used to use the span of the entire field expression e.f / struct pattern S { f } when comparing syntax contexts which wasn't correct since their constituent parts may come from different expansions, consider $e.f vs e.$f or S { $f, $g } with expn($f) ≠ expn($g). Now I use the most precise span where possible
  • 2nd commit: On unresolved imports, don't suggest similarly named hygienic names from different syntax contexts

r? @compiler-errors (feel free to reassign if you're short on time)

@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 Oct 9, 2023
@fmease fmease changed the title diagnostic suggestions: properly deal with hygienic names diagnostic: properly deal with hygienic names on unresolved fields and imports Oct 9, 2023
Comment on lines +1894 to 1895
PatKind::Struct(path, [], false) if path.span().eq_ctxt(pat.span) => {
(" { ", " }", path.span().shrink_to_hi().until(pat.span.shrink_to_hi()))
Copy link
Contributor

Choose a reason for hiding this comment

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

You can generalize this by looking at path.span().ancestor_within_span(pat.span).

Comment on lines +1903 to +1904
PatKind::Struct(..) if field.span.eq_ctxt(pat.span) => {
(", ", " }", field.span.shrink_to_hi().with_hi(pat.span.hi()))
Copy link
Contributor

Choose a reason for hiding this comment

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

You can generalize this by looking at field.span.ancestor_within_span(pat.span).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've already tried using find_ancestor_inside in some earlier version of this patch before opening a PR without a satisfying result if I remember correctly. However, let me check again tomorrow, I might've missed something.

Copy link
Member Author

@fmease fmease Oct 12, 2023

Choose a reason for hiding this comment

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

Hmm, so I'm almost certain that pat.span.FIND_ANCESTOR(field.span) with FIND_ANCESTOR ∊ {find_ancestor_inside, find_ancestor_in_same_ctxt} or just field.span isn't what we want here, eq_ctxt is conservative and correct (see next comment).

If the field and the struct patterns aren't in the same syntax context, there's not much we can do here structured-suggestion-wise without resorting to elaborate span_to_snippet hacks. If the field comes from a different expansion, then the struct pattern has to be of the form S { … $field … } or S { … $( … )… … }, meaning the field has to be a metavariable or a repetition but notably not a macro call like S { … m!(…) … } (ungrammatical). Therefore, there are two location were we could theoretically suggest the unmentioned fields:

  1. At the end of the struct pattern (~pat.span): However, we don't know if we should add a leading comma or not since we'd need to look at def-site (consider S { $( $fields/*,*/ )* }) and use-site (consider m!(field/*,*/)) which requires span_to_snippet I think and is ugly. The suggestion is marked MachineApplicable btw (we could ofc tweak that in this case to be MaybeIncorrect).
  2. At the end of the last field (~field.span)
    • However, the other fields (other than the last one) might come from a more "general" expansion.
    • The suggestion might be incorrect since we don't know the shape of the metavariable $field (etc.). Consider m!(field: _) where we don't know if the suggestion m!(field: _, missing: _) would be valid input for macro m

Copy link
Member Author

@fmease fmease Oct 12, 2023

Choose a reason for hiding this comment

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

I've noticed that eq_ctxt is not sufficient anyway for preventing invalid suggestions. For example:

struct S {
    a: (),
    b: (),
}

macro_rules! n {
    ($( $f:ident )?) => {
        let S { $( $f )? } = loop {};
    }
}

fn main() {
    n!(a);
}

Here, we suggest garbage:

error[E0027]: pattern does not mention field `b`
  --> krash.rs:17:13
   |
17 |         let S { $( $f )? } = loop {};
   |             ^^^^^^^^^^^^^^ missing field `b`
...
23 |     n!(a);
   |     ----- in this macro invocation
   |
   = note: this error originates in the macro `n` (in Nightly builds, run with -Z macro-backtrace for more info)
help: include the missing field in the pattern
   |
17 |         let S { $( $f, b } = loop {};
   |                      ~~~~~
help: if you don't care about this missing field, you can explicitly ignore it
   |
17 |         let S { $( $f, .. } = loop {};
   |                      ~~~~~~

On master, we don't but on the other hand we misclassifiy a and b to be inaccessible:

error: pattern requires `..` due to inaccessible fields
  --> krash.rs:17:13
   |
17 |         let S { $( $f )? } = loop {};
   |             ^^^^^^^^^^^^^^
...
23 |     n!(a);
   |     ----- in this macro invocation
   |
   = note: this error originates in the macro `n` (in Nightly builds, run with -Z macro-backtrace for more info)
help: ignore the inaccessible and unused fields
   |
17 |         let S { $( $f, .. )? } = loop {};
   |                      ++++

Seems like I still need to tweak some things :|.

@@ -2170,15 +2171,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
fn available_field_names(
&self,
variant: &'tcx ty::VariantDef,
expr: &hir::Expr<'_>,
span: Span,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you specify which span this is?

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2023
@bors
Copy link
Contributor

bors commented Nov 18, 2023

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

@Dylan-DPC
Copy link
Member

@fmease any updates on this? thanks

@oskgo
Copy link
Contributor

oskgo commented Aug 18, 2024

@fmease

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@oskgo oskgo closed this Aug 18, 2024
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

7 participants