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.

Sign up for GitHub

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

Jump to bottom

Updated error message for accidental uses of derive attribute as a crate attribute #89701

Merged
merged 1 commit into from
Dec 4, 2021
Merged

Updated error message for accidental uses of derive attribute as a crate attribute #89701

merged 1 commit into from
Dec 4, 2021

Conversation

tom7980
Copy link
Contributor

@tom7980 tom7980 commented Oct 9, 2021

This partially fixes the original issue #89566 by adding derive to the list of invalid crate attributes and then providing an updated error message however I'm not sure how to prevent the resolution error message from emitting without causing the compiler to just abort when it finds an invalid crate attribute (which I'd prefer not to do so we can find and emit other errors).

@petrochenkov I have been told you may have some insight on why it's emitting the resolution error though honestly I'm not sure if we need to worry about fixing it as long as we can provide the invalid crate attribute error also (which happens first anyway)

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tom7980 tom7980 marked this pull request as draft October 9, 2021 13:37
@oli-obk oli-obk 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 9, 2021
@tom7980 tom7980 marked this pull request as ready for review October 9, 2021 15:25
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

r? @petrochenkov

@petrochenkov petrochenkov 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 Oct 10, 2021
@petrochenkov
Copy link
Contributor

The issue is not about derive specifically, it's about any attribute macro from the standard prelude, e.g.

#![test] // error: cannot determine resolution for the attribute macro `test`

fn main() {}

and we certainly shouldn't hardcode the list of standard library macros in the compiler.

It is reported as a resolution error because it is a resolution error - we see some resolution candidate for the macro in the prelude, but we are not sure that it's the definitive result and at the same time cannot make any further progress either.
I don't think this PR is going into the right direction in general, instead we need to try improving expansion of macro attributes applied to the crate root, which is currently done through some hacks, and see whether it helps. I'll look into that in more detail.

@tom7980
Copy link
Contributor Author

tom7980 commented Oct 13, 2021

Great thank you for taking a look at this! I thought it might be the wrong way to go about it honestly and would prefer to be able to give more concrete advice on the resolution error so I'm happy to close this out.

Obviously #![derive()] was never meant to be applied to the crate root so I figured that checking for it would make sense considering we have a symbol available.

I'll look out for any fixes you might implement so I can learn from it, please do let me know in the meantime if I can help out!

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 13, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021
@bors

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

Blocked on #91313.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Dec 2, 2021
@petrochenkov
Copy link
Contributor

I don't think this is the right fix for derive, but we can keep the added suggestion to change the inner attribute to an outer one.

@petrochenkov petrochenkov 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 2, 2021
@tom7980 tom7980 force-pushed the issue-89566-fix branch 2 times, most recently from 281d399 to 9876569 Compare December 2, 2021 20:25
@tom7980
Copy link
Contributor Author

tom7980 commented Dec 2, 2021

No longer relying on BytePos to generate the replacement suggestion - the resolution error is still there but I'm not sure exactly what's causing that and I believe you are working on it.

I think this is more of a stopgap personally and if it would be better to fix it another way it may not even be worth changing but take a look and let me know what you think!

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov petrochenkov 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 2, 2021
@petrochenkov petrochenkov 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 3, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tom7980
Update invalid crate attributes, add help message
3827b64 
tidy run

update invalid crate attributes, improve error

update test outputs

de-capitalise error

update tests

Update invalid crate attributes, add help message

Update - generate span without using BytePos

Add correct dependancies

Update - generate suggestion without BytePos

Tidy run

update tests

Generate Suggestion without BytePos

Add all builtin attributes

add err builtin inner attr at top of crate

fix tests

add err builtin inner attr at top of crate

tidy fix

add err builtin inner attr at top of crate
@petrochenkov petrochenkov 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 4, 2021
@petrochenkov
Copy link
Contributor

@bors r

@bors
Copy link
Contributor

bors commented Dec 4, 2021

📌 Commit 3827b64 has been approved by petrochenkov

@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 4, 2021
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 4, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 4, 2021
@bors
Auto merge of rust-lang#91517 - matthiaskrgr:rollup-3fmp4go, r=matthi…
efec545 
…askrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#87054 (Add a `try_reduce` method to the Iterator trait)
 - rust-lang#89701 (Updated error message for accidental uses of derive attribute as a crate attribute)
 - rust-lang#90519 (Keep spans for generics in `#[derive(_)]` desugaring)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9f8e822 into rust-lang:master Dec 4, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

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
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.
8 participants
@tom7980 @rust-highfive @rust-log-analyzer @petrochenkov @bors @oli-obk @apiraino @rustbot