-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Rewrite inline
attribute parser to use new infrastructure and improve diagnostics for all parsed attributes
#138165
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -584,7 +546,6 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs { | |||
return OptimizeAttr::Default; | |||
}; | |||
|
|||
inline_span = Some(attr.span()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes #137950
e6dad01
to
2a707bf
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #138302) made this pull request unmergeable. Please resolve the merge conflicts. |
b7dda57
to
88a713a
Compare
This comment has been minimized.
This comment has been minimized.
@rustbot review |
This comment has been minimized.
This comment has been minimized.
ah shit, broke clippy 😭 |
| | ||
LL | #[rustc_confusables("invalid_meta_item")] | ||
| + + | ||
LL | #[rustc_confusables(invalid_meta_item)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rewording, arguably worse but consistent with other error messages with a consistent error code (as opposed to a dedicated diagnostic for this attribute only)
The `inline` attribute was malformed. | ||
|
||
Erroneous code example: | ||
|
||
```compile_fail,E0534 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer emitted because it's specific to inline. I kind of want to avoid those in favor of generic attribute related error codes
@@ -49,3 +48,29 @@ struct Stable; | |||
#[rustc_const_stable(feature = "stable_fn", since = "1.39.0")] // ok! | |||
const fn stable_fn() {} | |||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error code upgraded to represent the more generic issue of the structure of an attribute not matching the expected structure
Erroneous code example: | ||
|
||
```compile_fail,E0540 | ||
#[inline()] // error! should either have a single argument, or no parentheses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new generic error code for argument mismatch. 540 was not used.
| | ||
LL | #[deprecated(since = "a", note)] | ||
| ^^^^ | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^----^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much more detailed error messages
@@ -313,6 +303,16 @@ error[E0517]: attribute should be applied to a struct, enum, or union | |||
LL | #[repr(Rust)] impl S { } | |||
| ^^^^ ---------- not a struct, enum, or union | |||
|
|||
error: valid forms for the attribute are `#[inline(always|never)]` and `#[inline]` | |||
--> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:44:5 | |||
| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error message moved
| | ||
LL | fn barqux(#[rustc_force_inline] _x: u32) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a genuine regression, I think because check_attr now has to see this as a parsed attribute, let me fix that
@@ -8,7 +8,7 @@ extern crate std; | |||
// issue#97006 | |||
|
|||
macro_rules! m { ($attr_path: path) => { #[$attr_path] fn f() {} } } | |||
#[inline] | |||
#[attr = Inline(Hint)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@obi1kenobi is this one relevant for CSC? I'm going to upgrade more attributes like this I'm afraid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping! I think we don't SemVer-check based on it, but we do parse it so it'd be great to go back to emitting it as #[inline]
in rustdoc JSON in a separate PR if possible. cc @aDotInTheVoid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mhm, I messaged Alona today as well, since it's technically also a breaking change in rustdoc json. However, it's really annoying since I'll be making about a 100 separate breaking changes like this in the coming months.... and practically few people will care
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at the very least the debug representation is much more consistent nowadays. However, I think the solution is not to see this as a breaking change in the format, but just an unspecified part of the format you can't rely on at the moment until we stabilize it in a more structured way...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the solution is not to see this as a breaking change in the format, but just an unspecified part of the format you can't rely on at the moment
I think this is a fine mental model but I think it'll be frustrating to many users regardless, because we've given plenty of time to Hyrum's Law to kick in here.
Telling broken users "we're technically right, it was your assumptions that were wrong" will not make them any happier about it. So I think we should approach this situation with empathy for those users, and attempt to do what we can to minimize the negative impact as best as circumstances allow.
note, there are tests that tests the newly emitted lints during attr parsing. However, it works so well, none of them changed! They are emitted though, but exactly the same as they already were so it doesn't show up in the diff. Hard to check, but great! |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #142299) made this pull request unmergeable. Please resolve the merge conflicts. |
lint on duplicates during attribute parsing To do this we stuff them in the diagnostic context to be emitted after hir is constructed
This comment has been minimized.
This comment has been minimized.
ugh, the rebase |
r? @oli-obk
This PR:
Builds on top of #138164
Closes #137950