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

[option_option]: Fix duplicate diagnostics #12450

Merged
merged 2 commits into from
Mar 17, 2024

Conversation

cookie-s
Copy link
Contributor

@cookie-s cookie-s commented Mar 9, 2024

Relates to #12379

This option_option lint change skips checks against tys inside field_defs defined by external macro to prevent duplicate diagnostics to the same span ty by multiple Struct definitions.


changelog: [option_option]: Fix duplicate diagnostics

@rustbot
Copy link
Collaborator

rustbot commented Mar 9, 2024

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 9, 2024
@cookie-s
Copy link
Contributor Author

cookie-s commented Mar 9, 2024

The test failed because of #[derive(Serialize, Deserialize)] to struct Foo. There may be somewhere else where the in_external_macro guard can be added?

@cookie-s cookie-s marked this pull request as ready for review March 9, 2024 23:19
@llogiq
Copy link
Contributor

llogiq commented Mar 10, 2024

Yeah, those expansion contexts are sometimes weird. Perhaps the struct was marked but not the fields or something like this.

@cookie-s
Copy link
Contributor Author

I just added in_external_macro guard into check_field_def to make the test pass. If it's sufficient, I'd like this PR to be merged.

@llogiq
Copy link
Contributor

llogiq commented Mar 15, 2024

That raises the question whether to lint this in macros at all. If yes, then I'll gladly merge this. Otherwise we should use span.from_expansion() instead.

@cookie-s
Copy link
Contributor Author

@llogiq Thank you. That makes sense. I think it's better to skip all macros. I read this page too and updated to from_expansion.

@llogiq
Copy link
Contributor

llogiq commented Mar 17, 2024

Thank you for improving this lint!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 17, 2024

📌 Commit 9408c59 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 17, 2024

⌛ Testing commit 9408c59 with merge d202eb6...

@bors
Copy link
Contributor

bors commented Mar 17, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing d202eb6 to master...

@bors bors merged commit d202eb6 into rust-lang:master Mar 17, 2024
9 checks passed
@cookie-s cookie-s deleted the fix-optopt-duplicate-diags branch March 17, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants