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

Add suggestion to remove derive() if invoked macro is non-derive #109638

Merged
merged 9 commits into from
Apr 11, 2023

Conversation

NotStirred
Copy link
Contributor

@NotStirred NotStirred commented Mar 26, 2023

Adds to the existing expected derive macro, found {} error message:

help: remove the surrounding "derive()":
  --> $DIR/macro-path-prelude-fail-4.rs:1:3
   |
LL | #[derive(inline)]
   |   ^^^^^^^      ^

This suggestion will either fix the issue, in the case that the macro was valid, or provide a better error message if not

Not ready for merge yet, as the highlighted span is only valid for trivial formatting. Is there a nice way to get the parent span of the macro path within smart_resolve_macro_path?

Closes #109589

@rustbot
Copy link
Collaborator

rustbot commented Mar 26, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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 Mar 26, 2023
@workingjubilee
Copy link
Member

workingjubilee commented Mar 26, 2023

My trouble comes with knowing what the macro is invoked on, (function, struct), and sending the help message selectively based on that. Is there a way to get this information in smart_resolve_macro_path, or is my approach wrong and the error should be elsewhere?

Hm. What do you mean by "sending the help message selectively", exactly?

Though, in the cases this was known to be 100% correct, which I imagine selectivity might help, it would be nice if this had Applicability::MachineApplicable in those cases.

@NotStirred
Copy link
Contributor Author

Sorry for the confusion, originally I'd thought the help message was invalid for some cases, but it looks to be fine.

Based on tests all cases are valid. Though the Span adjustment code is definitely wrong as I mentioned in the original PR message

@NotStirred NotStirred marked this pull request as draft March 30, 2023 16:50
It's now split between two errors, one to remove the invalid derive macro
and one suggesting adding a new non-derive macro
@NotStirred NotStirred marked this pull request as ready for review March 30, 2023 20:54
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

apologies for the delay in getting back to this, left some more comments

compiler/rustc_resolve/src/macros.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/macros.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/macros.rs Outdated Show resolved Hide resolved
@@ -3,6 +3,14 @@ error: expected derive macro, found built-in attribute `inline`
|
LL | #[derive(inline)]
| ^^^^^^ not a derive macro
|
help: Remove from the surrounding `derive()`
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a new test case that has some of the trickier cases for this change? e.g. two derives, macro (to confirm it doesn't trigger), etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, this isn't valid. Was there something else you meant with macro?

macro_rules! foo {
    () => {
        Debug
    };
}

#[derive(foo!())]
struct V;

Copy link
Member

Choose a reason for hiding this comment

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

macro_rules! foo {
    ($foo:expr) => {
		#[derive($foo)]
		struct V;
    };
}

Something more like this?

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

I'll approve this since it's been a little while and it's in a good state. Thanks for working on this.

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 10, 2023

📌 Commit 668a629 has been approved by davidtwco

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 Apr 10, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 10, 2023
…avidtwco

Add suggestion to remove `derive()` if invoked macro is non-derive

Adds to the existing `expected derive macro, found {}` error message:
```
help: remove the surrounding "derive()":
  --> $DIR/macro-path-prelude-fail-4.rs:1:3
   |
LL | #[derive(inline)]
   |   ^^^^^^^      ^
```

This suggestion will either fix the issue, in the case that the macro was valid, or provide a better error message if not

Not ready for merge yet, as the highlighted span is only valid for trivial formatting. Is there a nice way to get the parent span of the macro path within `smart_resolve_macro_path`?

Closes rust-lang#109589
@bors
Copy link
Contributor

bors commented Apr 10, 2023

⌛ Testing commit 668a629 with merge 194a0bb...

@bors
Copy link
Contributor

bors commented Apr 11, 2023

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 194a0bb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 11, 2023
@bors bors merged commit 194a0bb into rust-lang:master Apr 11, 2023
@rustbot rustbot added this to the 1.70.0 milestone Apr 11, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (194a0bb): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.5% [-0.5%, -0.5%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)
4.1% [4.1%, 4.1%] 1
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.3% [-1.0%, 0.4%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.7% [-0.8%, -0.5%] 2
Improvements ✅
(secondary)
-5.1% [-6.3%, -3.9%] 2
All ❌✅ (primary) -0.7% [-0.8%, -0.5%] 2

@NotStirred NotStirred deleted the suggest/non-derive branch April 11, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

Suggest a code fix when an attribute macro is incorrectly used as a derive macro
8 participants