only set non-ADT derive error once per attribute, not per trait #44055
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oli-obk
reviewed
Aug 23, 2017
| @@ -428,8 +428,8 @@ impl<'a> TraitDef<'a> { | |||
| } | |||
| } | |||
| _ => { | |||
| cx.span_err(mitem.span, | |||
| "`derive` may only be applied to structs, enums and unions"); | |||
| // This not-an-ADT case is an error, but it should have | |||
There was a problem hiding this comment.
so this branch should be bug!("non-ADT derive should have been caught during expansion")
There was a problem hiding this comment.
Unfortunately, this will ICE: setting the span_err during expansion doesn't stop compilation from reaching this point. (But maybe it should??)
There was a problem hiding this comment.
Can you add a debug! statement here then? Also add a comment pointing at libsyntax/ext/expand.rs:MacroExpand::expand() for the handling of this case.
There was a problem hiding this comment.
Can you add a debug! statement here then?
Surprisingly, no: the libsyntax_ext crate doesn't have debug! in scope, and when I tried adding #[macro use] extern crate log; to lib.rs, we get "can't find crate for log". I don't understand what's going on there. (Added the expand filename comment pointer, though.)
|
You also need to update some compile-fail tests to your changes.
Click to expand
[00:47:25] [compile-fail] compile-fail/feature-gate/issue-43106-gating-of-derive-2.rs [00:47:25] [compile-fail] compile-fail/feature-gate/issue-43106-gating-of-derive.rs [00:47:25] [compile-fail] compile-fail/issue-36617.rs |
jseyfried
reviewed
Aug 23, 2017
src/libsyntax/ext/expand.rs
Outdated
| @@ -282,6 +282,22 @@ impl<'a, 'b> MacroExpander<'a, 'b> { | |||
| let expansion = self.expand_invoc(invoc, ext); | |||
| self.collect_invocations(expansion, &[]) | |||
| } else if let InvocationKind::Attr { attr: None, traits, item } = invoc.kind { | |||
| if let Annotatable::Item(ref item) = item { | |||
There was a problem hiding this comment.
Could we also error on Annotatable::TraitItem and Annotatable::ImplItem here and then also get rid of the other corresponding error in src/libsyntax_ext/deriving/generic/mod.rs? e.g.
let derive_allowed = match item {
Annotatable::Item(ref item) => match item.node {
ast::ItemKind::Struct(..) | ast::ItemKind::Enum(..) | ast::ItemKind::Union(..) => true,
_ => false,
_ => false,
};
if !derive_allowed {
// error
}
|
@zackmdavis looks good! r=me with @oli-obk's comment addressed. Derive expansion is still split somewhat between |
aidanhs
added
the
S-waiting-on-author
zackmdavis
force-pushed
the
condensed_non-ADT_derive_error
branch
from
6a9d0e1 to
9d4c171
Compare
|
Updated. Travis error appears spurious (just one macOS subjob failed, and its terminal log says that There are two potential concerns, though:
|
|
@zackmdavis Restarted the dummy macOS build, should be green in a few minutes. |
|
@zackmdavis do you think we should land this as is, or are you interested in perfecting it? If that, are you still interested in working on this? Just a friendly ping too make sure it isn't getting lost. |
|
@arielb1 My disposition is to merge as-is (the reservations bulleted in my previous comment being minor IMO), but if someone were to argue that it's important to preserve the exact error behavior specified in the original compile-fail/issue-43106-gating-of-derive-2.rs, I would respect that judgment, too. |
carols10cents
added
S-waiting-on-review
|
@jseyfried sounds like this is in your court for review. what do you think? |
|
ping @jseyfried @rust-lang/compiler for review please! |
estebank
reviewed
Sep 18, 2017
src/libsyntax/ext/expand.rs
Outdated
| .find(|attr| attr.check_name("derive")) | ||
| .expect("`derive` attribute should exist").span; | ||
| self.cx.span_err(span, | ||
| "`derive` may only be applied to structs, enums, \ |
There was a problem hiding this comment.
Can you keep the existing output and avoid the oxford comma? Not because I'm against it but I'd prefer to keep this change small (so it doesn't affect compile-fail/deriving-non-type.rs at all).
During the impl period I'm going to audit all the existing diagnostic output in order to identify current conventions to write them down. Doing this I fully expect to find inconsistent wording and will try to reach consensus for them.
There was a problem hiding this comment.
I trust that the community will affirm the correctness of the serial comma at that time.
| --> $DIR/issue-43927-non-ADT-derive.rs:13:1 | ||
| | | ||
| 13 | #![derive(Debug, PartialEq, Eq)] // should be an outer attribute! | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
This is a great example of a case where a note pointing out that this derive is being applied to the outer context instead of the struct immediately after would be beneficial. If you don't want to add that on this PR, could you please file a ticket for it?
It could be something along the lines of
error: `derive` may only be applied to structs, enums, and unions
--> $DIR/issue-43927-non-ADT-derive.rs:13:1
|
13 | #![derive(Debug, PartialEq, Eq)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ derive cannot be applied to outer mod
note: as it is written, the derive is applied to the outer scope, did you mean to apply it to the next element?
|
13 | #[derive(Debug, PartialEq, Eq)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| @@ -428,8 +428,8 @@ impl<'a> TraitDef<'a> { | |||
| } | |||
| } | |||
| _ => { | |||
| cx.span_err(mitem.span, | |||
| "`derive` may only be applied to structs, enums and unions"); | |||
| // This not-an-ADT case is an error, but it should have | |||
There was a problem hiding this comment.
Can you add a debug! statement here then? Also add a comment pointing at libsyntax/ext/expand.rs:MacroExpand::expand() for the handling of this case.
|
r=me with @estebank's comment addressed. |
A slight eccentricity of this change is that now non-ADT-derive errors prevent derive-macro-not-found errors from surfacing (see changes to the gating-of-derive compile-fail tests). Resolves rust-lang#43927.
zackmdavis
force-pushed
the
condensed_non-ADT_derive_error
branch
from
9d4c171 to
083f053
Compare
|
(force-pushed)
|
|
@bors r=jseyfried |
|
📌 Commit 083f053 has been approved by |
bors
added a commit
that referenced
this pull request
Sep 23, 2017
…seyfried only set non-ADT derive error once per attribute, not per trait I found the expansion code very hard to follow, leaving me unsure as to whether this might somehow be done better, but this patch does give us the behavior requested in #43927 (up to exact choice of span; here, it's the entire attribute, not just the `derive` token). (Note to GitHub robots: _resolves #43927_.) r? @jseyfried
|
☀️ Test successful - status-appveyor, status-travis |
|
💟 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I found the expansion code very hard to follow, leaving me unsure as to whether this might somehow be done better, but this patch does give us the behavior requested in #43927 (up to exact choice of span; here, it's the entire attribute, not just the
derivetoken).(Note to GitHub robots: resolves #43927.)
r? @jseyfried