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

Partially fix explicit_outlives_requirements lint in macros #106064

Merged
merged 3 commits into from
Dec 28, 2022

Conversation

lukas-code
Copy link
Member

@lukas-code lukas-code commented Dec 22, 2022

Show the suggestion if and only if the bounds are from the same source context.

fixes #106044
fixes #106063

Show the suggestion if and only if the bounds are from the same source context.
@rustbot
Copy link
Collaborator

rustbot commented Dec 22, 2022

r? @lcnr

(rustbot has picked a reviewer for you, use r? to override)

@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 Dec 22, 2022
@lukas-code lukas-code changed the title Fix explicit_outlives_requirements lint in macros Partially fix explicit_outlives_requirements lint in macros Dec 22, 2022
@lcnr
Copy link
Contributor

lcnr commented Dec 23, 2022

r? compiler (on vacation)

@rustbot rustbot assigned cjgillot and unassigned lcnr Dec 23, 2022
})
.unwrap_or(param_span.shrink_to_hi());
let span =
bounds.iter().fold(param_span.shrink_to_hi(), |span, bound| span.to(bound.span()));
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, the issue is that we are trying to reconstruct a span during lowering, which will mix spans coming from macro different expansions.
Fortunately, we know what SyntaxContext produces the :, it's GenericParam::colon_span.
So there are 2 cases here:

  • if colon_span is None, we should use param_span.shrink_to_hi();
  • if not, we should use it, and extend it with bound.span().find_ancestor_in_same_ctxt(colon_span.ctxt()).

Copy link
Member Author

@lukas-code lukas-code Dec 26, 2022

Choose a reason for hiding this comment

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

bound.span().find_ancestor_in_same_ctxt(colon_span.ctxt()) goes horribly wrong when the colon also comes from a macro parameter:

macro_rules! m {
($b:lifetime $colon:tt $a:lifetime) => {
struct Foo<$a, $b $colon $a>(&$a &$b ());
struct Bar<$a, $b>(&$a &$b ()) where $b $colon $a;
struct Baz<$a, $b>(&$a &$b ()) where (): Sized, $b $colon $a;
}
}
m!('b: 'a);

I changed this to search only in the "parent span", which is either the entire generics list for (normal) generic args or the impl Trait span.

compiler/rustc_lint/src/builtin.rs Outdated Show resolved Hide resolved
@cjgillot cjgillot 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 Dec 25, 2022
@lukas-code
Copy link
Member Author

I've addressed the review comments and this PR also fixes #106063 now. There are, however, still some remaining inconsistencies. See the FIXME at the bottom of the UI test for an example.

This happens, because the where clause span gets messed up in the parser here:

pub(super) fn parse_where_clause(&mut self) -> PResult<'a, WhereClause> {
let mut where_clause = WhereClause {
has_where_token: false,
predicates: Vec::new(),
span: self.prev_token.span.shrink_to_hi(),
};
if !self.eat_keyword(kw::Where) {
return Ok(where_clause);
}
where_clause.has_where_token = true;
let lo = self.prev_token.span;
// We are considering adding generics to the `where` keyword as an alternative higher-rank
// parameter syntax (as in `where<'a>` or `where<T>`. To avoid that being a breaking
// change we parse those generics now, but report an error.
if self.choose_generics_over_qpath(0) {
let generics = self.parse_generics()?;
self.struct_span_err(
generics.span,
"generic parameters on `where` clauses are reserved for future use",
)
.span_label(generics.span, "currently unsupported")
.emit();
}
loop {
let lo = self.token.span;
if self.check_lifetime() && self.look_ahead(1, |t| !t.is_like_plus()) {
let lifetime = self.expect_lifetime();
// Bounds starting with a colon are mandatory, but possibly empty.
self.expect(&token::Colon)?;
let bounds = self.parse_lt_param_bounds();
where_clause.predicates.push(ast::WherePredicate::RegionPredicate(
ast::WhereRegionPredicate {
span: lo.to(self.prev_token.span),
lifetime,
bounds,
},
));
} else if self.check_type() {
where_clause.predicates.push(self.parse_ty_where_predicate()?);
} else {
break;
}
let prev_token = self.prev_token.span;
let ate_comma = self.eat(&token::Comma);
if self.eat_keyword_noexpect(kw::Where) {
let msg = "cannot define duplicate `where` clauses on an item";
let mut err = self.struct_span_err(self.token.span, msg);
err.span_label(lo, "previous `where` clause starts here");
err.span_suggestion_verbose(
prev_token.shrink_to_hi().to(self.prev_token.span),
"consider joining the two `where` clauses into one",
",",
Applicability::MaybeIncorrect,
);
err.emit();
} else if !ate_comma {
break;
}
}
where_clause.span = lo.to(self.prev_token.span);
Ok(where_clause)
}

... due to incomplete handling of mixed macro expansions in Span::to:

if span_data.ctxt != end_data.ctxt {
if span_data.ctxt == SyntaxContext::root() {
return end;
} else if end_data.ctxt == SyntaxContext::root() {
return self;
}
// Both spans fall within a macro.
// FIXME(estebank): check if it is the *same* macro.
}

This is presumably a problem elsewhere too and should probably be addressed in general (in a different PR) instead of adding more special casing here.

@rustbot review

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

I'm not sure I understand what is the issue with your FIXME.
Anyway, this is a great improvement!
@bors r+

@bors
Copy link
Contributor

bors commented Dec 27, 2022

📌 Commit 1eba6c4 has been approved by cjgillot

It is now in the queue for this repository.

@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 Dec 27, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 27, 2022
…mpiler-errors

Rollup of 9 pull requests

Successful merges:

 - rust-lang#103718 (More inference-friendly API for lazy)
 - rust-lang#105765 (Detect likely `.` -> `..` typo in method calls)
 - rust-lang#105852 (Suggest rewriting a malformed hex literal if we expect a float)
 - rust-lang#105965 (Provide local extern function arg names)
 - rust-lang#106064 (Partially fix `explicit_outlives_requirements` lint in macros)
 - rust-lang#106179 (Fix a formatting error in Iterator::for_each docs)
 - rust-lang#106181 (Fix doc comment parsing description in book)
 - rust-lang#106187 (Update the documentation of `Vec` to use `extend(array)` instead of `extend(array.iter().copied())`)
 - rust-lang#106189 (Fix UnsafeCell Documentation Spelling Error)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 18bf19c into rust-lang:master Dec 28, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 28, 2022
@lukas-code lukas-code deleted the outlives-macro branch December 28, 2022 13:30
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
5 participants