simplify_type: don't simplify projections
#92721
Closed
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
rustbot
added
the
T-compiler
rust-highfive
added
the
S-waiting-on-review
lcnr
force-pushed
the
simplify_type-projections
branch
2 times, most recently
from
3106bea
to
7323d0d
Compare
lcnr
force-pushed
the
simplify_type-projections
branch
from
7323d0d
to
13f7edc
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 13f7edc with merge be79c364343564bbcc7f36d4b3d932f2b49f3482... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
nikomatsakis
requested changes
Jan 10, 2022
| @@ -60,29 +60,23 @@ pub enum StripReferences { | |||
| No, | |||
| } | |||
|
|
|||
| /// Tries to simplify a type by only returning the outermost injective¹ layer, if one exists. | |||
| /// Tries to simplify a type by only returning the outermost layer of an inductive type, if one exists. | |||
There was a problem hiding this comment.
I don't really know what "inductive" type means here, and I expect other readers won't either =)
| /// | ||
| /// When using `SimplifyParams::Yes`, we still return a simplified type for params and projections², | ||
| /// the reasoning for this can be seen at the places doing this. | ||
| /// When using `SimplifyParams::Yes`, we still return a simplified type for params. |
There was a problem hiding this comment.
I wonder if a better name for this would be TreatParamsAsPlaceholders-- I've been trying to push the "placeholder" terminology for "universally bound free variable".
There was a problem hiding this comment.
Or something like
enum TreatParams {
AsPlaceholderTypes,
AsBoundTypes,
}
Comment on lines
+129
to
+136
| // Returning a simplified for projections can end up being unsound as it would | ||
| // stop us from considering not consider non blanket impls for `<_ as Trait>::Assoc` even | ||
| // though `_` can be inferred to a concrete type later at which point a concrete impl | ||
| // could actually apply. | ||
| // | ||
| // We could return `Some` here in cases where the input type is fully normalized, but | ||
| // I (@lcnr) don't know how to guarantee that. | ||
| None |
There was a problem hiding this comment.
Suggested change
| // Returning a simplified for projections can end up being unsound as it would | |
| // stop us from considering not consider non blanket impls for `<_ as Trait>::Assoc` even | |
| // though `_` can be inferred to a concrete type later at which point a concrete impl | |
| // could actually apply. | |
| // | |
| // We could return `Some` here in cases where the input type is fully normalized, but | |
| // I (@lcnr) don't know how to guarantee that. | |
| None | |
| // Returning a simplified for projections can end up being unsound. Consider this example: | |
| // | |
| // ```rust | |
| // trait Foo { } | |
| // impl Foo for u32 { } | |
| // | |
| // trait Bar { type Assoc; } | |
| // impl Bar for f32 { type Assoc = u32; } | |
| // impl Bar for f64 { type Assoc = u64; } | |
| // ``` | |
| // If we are matching `<_ as Bar>::Assoc` against `Foo`, we might rule out the impl even though `_` could be inferred to `f32` and hence it might match. | |
| None |
|
☀️ Try build successful - checks-actions |
|
Queued be79c364343564bbcc7f36d4b3d932f2b49f3482 with parent df035a3, future comparison URL. |
|
Finished benchmarking commit (be79c364343564bbcc7f36d4b3d932f2b49f3482): comparison url. Summary: This change led to very large relevant regressions 😿 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
rustbot
added
perf-regression
|
triage: returning to author to address comments |
rustbot
added
S-waiting-on-author
is this expected? |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Mar 3, 2022
improve comments for `simplify_type` Should now correctly describe what's going on. Experimented with checking the invariant for projections but that ended up requiring fairly involved changes. I assume that it is not possible to get unsoundness here, at least for now and I can pretty much guarantee that it's impossible to trigger it by accident. r? ````@nikomatsakis```` cc rust-lang#92721
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Mar 3, 2022
improve comments for `simplify_type` Should now correctly describe what's going on. Experimented with checking the invariant for projections but that ended up requiring fairly involved changes. I assume that it is not possible to get unsoundness here, at least for now and I can pretty much guarantee that it's impossible to trigger it by accident. r? `````@nikomatsakis````` cc rust-lang#92721
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.
cc #86986 (comment)
r? @nikomatsakis