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

Fix error warning span for issue12045 #12125

Merged
merged 3 commits into from Jan 19, 2024
Merged

Conversation

cocodery
Copy link
Contributor

fixes Issue#12045

In issue#12045, unexpected warning span occurs on attribute #[derive(typed_builder::TypedBuilder)], actually the warning should underline _lifetime.

In the source code we can find that the original intend is to warning on ident.span, but in this case, stmt.span is unequal with ident.span. So, fix the nit here is fine.

Besides, ident.span have an accurate range than stmt.span.

changelog: [no_effect_underscore_binding]: correct warning span

@rustbot
Copy link
Collaborator

rustbot commented Jan 11, 2024

r? @Manishearth

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 11, 2024
@cocodery
Copy link
Contributor Author

r? @xFrednet

@rustbot rustbot assigned xFrednet and unassigned Manishearth Jan 11, 2024
@xFrednet
Copy link
Member

Looks good to me, I love that the test code looks familiar ^^

Thank you for the update :D

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 11, 2024

📌 Commit 7c389ac has been approved by xFrednet

It is now in the queue for this repository.

@xFrednet
Copy link
Member

@bors r-

@bors
Copy link
Collaborator

bors commented Jan 11, 2024

⌛ Testing commit 7c389ac with merge 5e0939f...

bors added a commit that referenced this pull request Jan 11, 2024
Fix error warning span for issue12045

fixes [Issue#12045](#12045)

In issue#12045, unexpected warning span occurs on attribute `#[derive(typed_builder::TypedBuilder)]`, actually the warning should underline `_lifetime`.

In the source code we can find that the original intend is to warning on `ident.span`, but in this case, `stmt.span` is unequal with `ident.span`. So, fix the nit here is fine.

Besides, `ident.span` have an accurate range than `stmt.span`.

changelog: [`no_effect_underscore_binding`]: correct warning span
@xFrednet
Copy link
Member

Wait, I like the change to the emission span, but I believe that it will not resolve the linked issue. The problem is, that the lint doesn't notice that the thing it lints, is inside derived code. The lint level is determined at the emission node, not in the span, so this change will still leave the FP. This is a nice update either way, but to solve this issue you probably need to add a check with any_parent_is_automatically_derived and also add a test with derived code.

@bors
Copy link
Collaborator

bors commented Jan 11, 2024

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: 5e0939f (5e0939f259821575997ac57f3287fc87a7ead452)

@cocodery
Copy link
Contributor Author

cocodery commented Jan 12, 2024

I have already fixed the addition question locally by add !any_parent_is_automatically_derived(cx.tcx, local.hir_id) to prevent derive expansion code.

But sorry that the test seems not easy to add. I have figured out the issue is generate by derive node expansion, through marco parse_macro_input I think. So writiing some proc_macro_derive attribute uses marco parse_macro_input and use them in struct field maybe fine.

If directly adding related issue code with typed-builder = "0.18" into toml file, and copy codes to test file, a can't find crate error emitted I havn't figured out. Or maybe [lib] proc-macro = true [dependencies.proc-macro2] version = "1" kind of things should be added into toml file, but too complex. Both will change toml file, I think maybe it's not suitable to modify toml file.

Is is available to clarity this issue in test file comment instead of a test code? :) @xFrednet

@xFrednet
Copy link
Member

Sorry for the late response the end of my winter semester is killing me 😅

I have already fixed the addition question locally by add !any_parent_is_automatically_derived(cx.tcx, local.hir_id) to prevent derive expansion code.

I don't see this change in this PR, have you done that part in a previous one?

Regarding testing, this is sadly complicated for these kinds of macros. We have util for other macros, but apparently not derives. I checked the test for another lint and that one doesn't have tests for the derive case, so I think it's safe to close the issue if the any_parent_is_automatically_derived check is present. :)

@cocodery
Copy link
Contributor Author

hahhaha, no worries, it's fine.

I fixed in one local commit, I haven't push yet beacuse of the complexity of test and was waiting your reply last several days.

@xFrednet
Copy link
Member

Thanks, looks good to me :)

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 19, 2024

📌 Commit 73d7ce6 has been approved by xFrednet

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jan 19, 2024

⌛ Testing commit 73d7ce6 with merge 989ce17...

@bors
Copy link
Collaborator

bors commented Jan 19, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 989ce17 to master...

@bors bors merged commit 989ce17 into rust-lang:master Jan 19, 2024
5 checks passed
@cocodery cocodery deleted the issue12045 branch January 28, 2024 05:42
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.

no_effect_underscore_binding: FP on field inside derive macros
5 participants