-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Record semantic types for all syntactic types in bodies #44633
Conversation
src/librustc_privacy/lib.rs
Outdated
} | ||
} else { | ||
// Types in signatures. | ||
if rustc_typeck::hir_ty_to_ty(self.tcx, hir_ty).visit_with(self) { |
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 very ineffective.
What I would actually like is to resurrect global ast_ty_to_ty_cache, but I don't know how to do it in the new query-oriented world.
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.
It wouldn't be global, it would be per-item and it wouldn't be a cache but an eagerly computed map.
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.
eagerly computed? do you mean lazilly computed?
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.
so .. why can't we put this information into the table? just because the type checker doesn't happen to include it now? It seems like we could begin the type-checking process by invoking hir_ty_to_ty
on the argument types in the same way we do here. Not sure if that would be better.
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 mean eagerly computed per-item, because you have to return the entire map.
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.
ah, ok, yes.
☔ The latest upstream changes (presumably #44678) made this pull request unmergeable. Please resolve the merge conflicts. |
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 looks fine as far as it goes; I agree it doesn't feel necessarily very elegant, somehow, although having the information be part of the typeck tables seems... like a pretty reasonable place to put it...
In case it wasn't clear, r=me -- but needs rebase. I think maybe we'll revisit this over time and try to find a nicer way to package it up, but this seems like it should work for now! |
@bors r=nikomatsakis |
📌 Commit 419069d has been approved by |
Record semantic types for all syntactic types in bodies ... and use recorded types in type privacy checking (types are recorded after inference, so there are no `_`s left). Also use `hir_ty_to_ty` for types in signatures in type privacy checking. This could also be potentially useful for save-analysis and diagnostics. Fixes #42125 (comment) r? @eddyb
☀️ Test successful - status-appveyor, status-travis |
Type privacy polishing Various preparations before implementing rust-lang/rfcs#2145 containing final minor breaking changes (mostly for unstable code or code using `allow(private_in_public)`). (Continuation of #42125, #44633 and #41332.) It would be good to run crater on this. r? @eddyb
... and use recorded types in type privacy checking (types are recorded after inference, so there are no
_
s left).Also use
hir_ty_to_ty
for types in signatures in type privacy checking.This could also be potentially useful for save-analysis and diagnostics.
Fixes #42125 (comment)
r? @eddyb