-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
TypeId: use a (v0) mangled type to remain sound in the face of hash collisions. #95845
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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 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 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 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 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 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 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 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 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 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 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 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -266,6 +266,7 @@ symbols! { | |
Ty, | ||
TyCtxt, | ||
TyKind, | ||
TypeId, | ||
Unknown, | ||
UnsafeArg, | ||
Vec, | ||
|
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer this helper to be in the
interpret
module, notconst_eval
-- it is needed for all interpreter instances after all,const_eval
is meant for specifically the CTFE instance of the general interpreter infrastruture.EDIT: It is needed also for regular codegen, but I think my point remains --
const_eval
should be for things that are specific for CTFE.interpret
should not usually import anything fromconst_eval
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're also creating your own
ecx
here, and returning this as aConstValue
-- why all that?I guess an alternative might be to treat this like
vtable_allocation
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type_name::alloc_type_name
also seems similar in spirit?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooh I see, I was modelling it after
const_caller_location
but that's only here because it's a query instead of an intrinsic. Should it have its own module underinterpret::intrinsics
likecaller_location
does?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, note in particular that the
caller_location
implementation ininterpret
does not call theconst_eval
code. It is not a nullary intrinsic so it takes slightly different paths. Though probably theconst_caller_location
query should be made more like the vtable query, which is not insideconst_eval
either.Yeah I guess that would make sense.
It would almost make sense to move these queries for intrinsics that are implemented via interpreter stuff but also used for codegen in a separate module
rustc_const_eval::intrinsics
, i.e., outsideinterpret
. @oli-obk any thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at it,
caller_location
actually has aimpl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M>
block so having that insideinterpret
makes sense.type_name
, OTOH, doesn't really do anything with the interpreter, it is very much like thevtable
code (but the two are in totally different locations, which we should probably fix). "give me the vtable for type T" is not technically an intrinsic but not very different, either, is it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think cleaning this up is orthogonal.
Except for
mplace_field
we don't even need anInterpCx
and can just create and intern theAllocation
s directly. If we added more convenience operations for writing data of a specific struct to anAllocation
we can probably stop usingInterpCx
forcaller_location
, too.Imo we should just merge this PR and then work on cleaning these up now that we have multiple intrinsics to clean up in a similar way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree cleaning up the existing stuff is a separate task.
But we shouldn't have anything load-bearing inside the
const_eval
module, so I do think that this code should be moved somewhere else before landing. I don't care strongly which of the existing precedent it follows, but none of it has load-bearing code in const_eval, so let's not add yet another different style of implementing such an intrinsic. :)(Even
caller_location
, which was mentioned as a template, has its load-bearing code ininterpret::intrinsics::caller_location
.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still relevant, should move this function.