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

add known-bug tests for derive failure to detect packed repr #121025

Merged
merged 1 commit into from
Mar 8, 2024
Merged

add known-bug tests for derive failure to detect packed repr #121025

merged 1 commit into from
Mar 8, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 13, 2024 •
edited
Loading

We only taint if it was a normal item. Modules and imports are untouched. Tainting them needs to be done differently, and it's unclear if that would be useful or desirable. If we just taint them into Res::Err, we end up losing some duplicate name messages in the presence of other resolution errors.

r? @petrochenkov

@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 Feb 13, 2024
@bors

This comment was marked as resolved.

Sign in to view
@bors

This comment was marked as resolved.

Sign in to view
@petrochenkov
Copy link
Contributor

What will happen in a case like this? Will it still ICE?

#[derive(PartialEq)]
#[proc_macro_attribute_that_generates_repr_packed]
struct Dealigned<T>(u8, T);

Built-in macros are still macros, they still see their input as tokens (well, maybe as AST, but that is supposed to be an unobservable optimization).
I'm not sure we should make assumptions about their output that are strong enough to produce ICEs when something goes wrong.
@rustbot author

@rustbot rustbot 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 Mar 4, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 5, 2024

oh wow this has been an issue since the repr(packed) special casing in derives was added: https://github.com/rust-lang/rust/pull/44884/files#diff-b4f496d9af443165c646c4418ab441725f3f46770121cec0b9c2fab246f64724R412

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 5, 2024

I'm not sure we should make assumptions about their output that are strong enough to produce ICEs when something goes wrong.

I don't see any way around that (well we could error), but the fundamental limitation is there as long as we don't expand proc macros attributes before we run the derive.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 5, 2024

@rustbot review

I added a known-bug ICE test and marked this PR as not fixing the issue anymore.

I think the diagnostic changes are still an improvement, so we may want to land it regardless?

@rustbot rustbot 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 Mar 5, 2024
@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 5, 2024 •
edited
Loading

Yeah, the diagnostics are improved, but I'm not sure what are the full implications of updating the resolution here.
Resolutions for items are "definite" and not expected to change once set (unlike e.g. glob imports).

So the old resolution may stuck in some places, and the new resolution in other places, and the mismatches could cause ICEs in places like import validation.
See for example #57185, that fixed one such mismatch.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 5, 2024

Yea, makes sense. Do you have any ideas what we could do to taint anything in case there are name collisions? If not, let's just close this and address the ICE separately

@oli-obk oli-obk mentioned this pull request Mar 5, 2024
Open
@petrochenkov
Copy link
Contributor

I need to think.

@petrochenkov
Copy link
Contributor

Let's just not do this, the improvements are probably not worth the effort.
(But keep the added tests.)
@rustbot author

@rustbot rustbot 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 Mar 7, 2024
@oli-obk
Add known-bug tests for derive(PartialEq) mismatches with `#[repr(p…
bed9d1f 
…acked)]` attributes that are not visible before macro expansion
@oli-obk oli-obk force-pushed the taint_after_errors branch from 1f956fe to bed9d1f Compare March 7, 2024 21:40
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 7, 2024

Makes sense, thanks for checking.

Kept the tests, removed the rest.

@rustbot review

@rustbot rustbot 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 Mar 7, 2024
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 8, 2024

📌 Commit bed9d1f has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 8, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 8, 2024
@oli-obk oli-obk changed the title Taint resolution if there were multiple items of the same name add known-bug tests for derive failure to detect packed repr Mar 8, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 8, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
@bors
Auto merge of rust-lang#122190 - matthiaskrgr:rollup-9ol4y30, r=matth…
28e8274 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#121025 (add known-bug tests for derive failure to detect packed repr)
 - rust-lang#121194 (Refactor pre-getopts command line argument handling)
 - rust-lang#121563 (Use `ControlFlow` in visitors.)
 - rust-lang#122173 (Don't ICE in CTFE if raw/fn-ptr types differ)
 - rust-lang#122175 (Bless tidy issues order)
 - rust-lang#122179 (rustc: Fix typo)
 - rust-lang#122181 (Fix crash in internal late lint checking)
 - rust-lang#122183 (interpret: update comment about read_discriminant on uninhabited variants)

Failed merges:

 - rust-lang#122076 (Tweak the way we protect in-place function arguments in interpreters)
 - rust-lang#122132 (Diagnostic renaming 3)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
@bors
Auto merge of rust-lang#122190 - matthiaskrgr:rollup-9ol4y30, r=matth…
a655e64 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#121025 (add known-bug tests for derive failure to detect packed repr)
 - rust-lang#121194 (Refactor pre-getopts command line argument handling)
 - rust-lang#121563 (Use `ControlFlow` in visitors.)
 - rust-lang#122173 (Don't ICE in CTFE if raw/fn-ptr types differ)
 - rust-lang#122175 (Bless tidy issues order)
 - rust-lang#122179 (rustc: Fix typo)
 - rust-lang#122181 (Fix crash in internal late lint checking)
 - rust-lang#122183 (interpret: update comment about read_discriminant on uninhabited variants)

Failed merges:

 - rust-lang#122076 (Tweak the way we protect in-place function arguments in interpreters)
 - rust-lang#122132 (Diagnostic renaming 3)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 075f1c3 into rust-lang:master Mar 8, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
@rust-timer
Unrolled build for rust-lang#121025
3e513c4 
Rollup merge of rust-lang#121025 - oli-obk:taint_after_errors, r=petrochenkov

add known-bug tests for derive failure to detect packed repr

We only taint if it was a normal item. Modules and imports are untouched. Tainting them needs to be done differently, and it's unclear if that would be useful or desirable. If we just taint them into `Res::Err`, we end up losing some duplicate name messages *in the presence of other resolution errors*.

r? `@petrochenkov`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.78.0
Development

Successfully merging this pull request may close these issues.
4 participants
@oli-obk @bors @petrochenkov @rustbot