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.

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

privacy: Relax some asserts on effective visibilities #104546

Closed
wants to merge 1 commit into from

Conversation

petrochenkov
Copy link
Contributor

The first invariant is hard to enforce in presence of errors.

The second invariant is expensive to enforce fully precisely, so it no longer makes sense given that we were trying to enforce it for performance.

Fixes #104249.
Fixes #104539.
(Both ICEs happen only if the compiler is built with debug assertions.)

The first invariant is hard to enforce in presence of errors.

The second invariant is expensive to enforce fully precisely, so it no longer makes sense given that we were trying to enforce it for performance
@rustbot
Copy link
Collaborator

rustbot commented Nov 17, 2022

r? @lcnr

(rustbot has picked a reviewer for you, use r? to override)

@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 Nov 17, 2022
@lcnr
Copy link
Contributor

lcnr commented Nov 21, 2022

looks sensible. still want someone who knows about this code look at this as there might be a good way to keep these asserts if they're useful

r? compiler

@rustbot rustbot assigned eholk and unassigned lcnr Nov 21, 2022
@eholk
Copy link
Contributor

eholk commented Nov 21, 2022

Looks reasonable to me, but I don't think I count as someone who knows this code so I'll reroll too.

r? compiler

@rustbot rustbot assigned compiler-errors and unassigned eholk Nov 21, 2022
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with comment addressed or not.

@@ -167,7 +166,7 @@ impl EffectiveVisibilities {
// and all effective visibilities are larger or equal than private visibility.
let private_vis = Visibility::Restricted(tcx.parent_module_from_def_id(def_id));
let span = tcx.def_span(def_id.to_def_id());
if !ev.direct.is_at_least(private_vis, tcx) {
if !ev.direct.is_at_least(private_vis, tcx) && tcx.sess.has_errors().is_none() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to delay span bug here and the assert you removed below?

@compiler-errors compiler-errors 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 Nov 22, 2022
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Nov 23, 2022

Superseded by #104602 which actually fixes the underlying issue for #104249.
That PR is larger and not suitable for a backport, but these fixes probably don't need to be backported because they only affect custom compilers built with debug assertions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
5 participants