Enforce a whitelist on the constant primitive types allowed in patterns. #70872
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
rust-highfive
added
the
S-waiting-on-review
|
r? @pnkfelix |
|
@bors try (for check-only crater) |
|
⌛ Trying commit 458bae5 with merge 3ae4c050387df9f7fbb379ee90ce8bf05e566870... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☀️ Try build successful - checks-azure |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
Due to #70889, we might need to do a completely separate check for const generics, so we don't have to be as strict on pattern But I'm curious what the results are regardless. |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
Most regressions seem to stem from However, both of those are similar, in that they're raw pointers with an integral value:
Maybe we should only disallow data/ |
|
Like you said
|
joelpalmer
added
S-waiting-on-review
pnkfelix
reviewed
Apr 28, 2020
| let mut leaf_ty = cv.ty; | ||
|
|
||
| // HACK(eddyb) workaround missing reference destructuring. | ||
| loop { |
There was a problem hiding this comment.
I'm a little surprised to see there not a pre-existing method that does this traversal to find the content underlying a ref/array/slice? (Is the presupposition that we do not want such a method because any such traversal is almost certainly a hack, and should be annotated as such, as eddyb as done here?)
Since I misread the above when I first glanced on it, I'll just point out explicitly that the quoted code has occurrences of numbers that end up cast to both null ( (This doesn't contradict or argue against anything eddyb said. I just wanted to make it clear, since at first I was wondering if we could do something as narrow as only matching on null raw pointers.) |
|
@eddyb what do you want to do here? I anticipate that we could not land this code as is; I would expect at least a warning cycle for a change of this magnitude, no? |
pnkfelix
removed
the
S-waiting-on-review
pnkfelix
added
the
S-waiting-on-author
|
@pnkfelix I don't expect to be able to work on this any time soon, so feel free to close the PR and/or take over it. Given everything we've learned I think that a primarily value-based approach for primitives is necessary, although I'm not sure what to do about constants we can't evaluate ahead of time, do we disallow pointer leaves in them based only on "some values aren't acceptable"? The value-based approach extends to Pointers are |
joelpalmer
added
S-waiting-on-author
Elinvynia
added
S-waiting-on-author
|
Closing this. Thanks |
Dylan-DPC-zz
removed
the
S-waiting-on-author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The primary effect of this is that it bans function and raw pointers from being used in constants that show up in patterns (see #70861).
This PR is for Crater only, and I suspect the fallout might be bad enough that we can't ever do this.
If we can't change the rules for patterns, we need to come up with separate rules for
constgenerics, that take into account the "structurally matchable" property for user-defined ADTs, but otherwise has a strict whitelist of primitive types. And I suppose we don't need to do a value-based analysis forconstgenerics, we can be stricter on purpose.cc @pnkfelix @varkor @yodaldevoid