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

Merge check_mod_impl_wf and check_mod_type_wf #121154

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 15, 2024

This still causes some funny diagnostics, but I'm not sure they can be fixed without a larger change, which I'd like to avoid here.

Reducing the number of times we iterate over the same items at this high level helps avoid parallel-compiler bottlenecks.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 15, 2024

@bors try @rust-timer queue

@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2024

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Feb 15, 2024
@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 15, 2024
@bors
Copy link
Contributor

bors commented Feb 15, 2024

⌛ Trying commit ade1cd7 with merge 24e3e17...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 15, 2024
Merge `check_mod_impl_wf` and `check_mod_type_wf`

This still causes some funny diagnostics, but I'm not sure they can be fixed without a larger change, which I'd like to avoid here.
Comment on lines -173 to +183
// Check impls constrain their parameters
let res =
tcx.hir().try_par_for_each_module(|module| tcx.ensure().check_mod_impl_wf(module));
tcx.hir().par_for_each_module(|module| {
let _ = tcx.ensure().check_mod_type_wf(module);
});

for &trait_def_id in tcx.all_local_trait_impls(()).keys() {
let _ = tcx.ensure().coherent_trait(trait_def_id);
}
// these queries are executed for side-effects (error reporting):
let _ = tcx.ensure().crate_inherent_impls(());
let _ = tcx.ensure().crate_inherent_impls_overlap_check(());
res
})?;
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just removing this early abort causes annoying diagnostics that are hard to make good. So what I did instead was to inline the check_mod_impl_wf checks directly into the individual check_well_formed of impl items and early abort per item. So that only items with errors in impl_wf checks don't get their type_wf run, but other items do

Comment on lines 2601 to 2608
if let GenericParamKind::Lifetime = param.kind {
// Record lifetime res, so lowering knows there is something fishy.
self.record_lifetime_param(param.id, LifetimeRes::Error);
}
let rib = match param.kind {
GenericParamKind::Lifetime => {
// Record lifetime res, so lowering knows there is something fishy.
self.record_lifetime_param(param.id, LifetimeRes::Error);
continue;
}
GenericParamKind::Type { .. } => &mut function_type_rib,
GenericParamKind::Const { .. } => &mut function_value_rib,
};

self.r.record_partial_res(param.id, PartialRes::new(Res::Err));
rib.bindings.insert(ident, Res::Err);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this change we'd get a "duplicate generic param T" error followed by an "unused generic param T" error for

struct Foo<T, T>(T);

The latter error makes no sense, so I turned the second T into a Res::Err, which hides follow-up errors

Copy link
Contributor

Choose a reason for hiding this comment

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

Add as a comment too.

Comment on lines 75 to 81
if impl_source.has_infer() {
infcx.tcx.dcx().has_errors().unwrap();
return Err(CodegenObligationError::FulfillmentError);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused lifetimes on an impl get replaced with inference vars, but never resolved, causing the return value of a query to contain inference vars, which is

  1. bad
  2. ICEs in stable hashing (good!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put this comment as a comment in the code?

Comment on lines +7 to +11
error[E0282]: type annotations needed
--> $DIR/issue-87340.rs:11:23
|
LL | fn f() -> Self::I {}
| ^^ cannot infer type for type parameter `T`
Copy link
Contributor Author

@oli-obk oli-obk Feb 15, 2024

Choose a reason for hiding this comment

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

annoying to prevent, but I'll figure it out, preferably in a follow-up PR

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 15, 2024

☀️ Try build successful - checks-actions
Build commit: 24e3e17 (24e3e17dc6525afe6e8d963e58ec5bb1e8a62f5e)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (24e3e17): comparison URL.

Overall result: ✅ improvements - no action needed

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 may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.5% [-6.7%, -0.2%] 5
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.7% [2.7%, 2.7%] 1
Regressions ❌
(secondary)
1.5% [1.5%, 1.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.7% [2.7%, 2.7%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.1% [1.1%, 1.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.1% [-4.9%, -1.3%] 2
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 638.252s -> 637.587s (-0.10%)
Artifact size: 306.28 MiB -> 306.30 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 15, 2024
@bors
Copy link
Contributor

bors commented Feb 20, 2024

☔ The latest upstream changes (presumably #121327) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Feb 21, 2024

☔ The latest upstream changes (presumably #121356) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Feb 21, 2024

☔ The latest upstream changes (presumably #121400) made this pull request unmergeable. Please resolve the merge conflicts.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2024
Merge `collect_mod_item_types` query into `check_well_formed`

follow-up to rust-lang#121154

r? `@ghost`
@bors
Copy link
Contributor

bors commented Feb 26, 2024

☔ The latest upstream changes (presumably #119106) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Mar 2, 2024

☔ The latest upstream changes (presumably #121870) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 7, 2024
@bors bors merged commit 8c9a75b into rust-lang:master Mar 7, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 7, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2024
Merge `collect_mod_item_types` query into `check_well_formed`

follow-up to rust-lang#121154

this removes more potential parallel-compiler bottlenecks and moves diagnostics for the same items next to each other, instead of grouping diagnostics by analysis kind
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8c9a75b): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.6% [-2.6%, -2.6%] 1
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.2% [-2.2%, -2.2%] 2
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 647.185s -> 648.501s (0.20%)
Artifact size: 172.72 MiB -> 172.69 MiB (-0.02%)

@oli-obk oli-obk deleted the track_errors11 branch March 7, 2024 13:48
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
Merge `collect_mod_item_types` query into `check_well_formed`

follow-up to rust-lang#121154

this removes more potential parallel-compiler bottlenecks and moves diagnostics for the same items next to each other, instead of grouping diagnostics by analysis kind
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
Merge `collect_mod_item_types` query into `check_well_formed`

follow-up to rust-lang#121154

this removes more potential parallel-compiler bottlenecks and moves diagnostics for the same items next to each other, instead of grouping diagnostics by analysis kind
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 9, 2024
Merge `collect_mod_item_types` query into `check_well_formed`

follow-up to rust-lang/rust#121154

this removes more potential parallel-compiler bottlenecks and moves diagnostics for the same items next to each other, instead of grouping diagnostics by analysis kind
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2024
Silence some follow-up errors on trait impls in case the trait has conflicting or otherwise incoherent impls

fixes rust-lang#123292

Also removes a bunch of extra diagnostics that were introduced in rust-lang#121154 and rust-lang#120558
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2024
Silence some follow-up errors on trait impls in case the trait has conflicting or otherwise incoherent impls

fixes rust-lang#123292

Also removes a bunch of extra diagnostics that were introduced in rust-lang#121154 and rust-lang#120558
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 17, 2024
Silence some follow-up errors on trait impls in case the trait has conflicting or otherwise incoherent impls

fixes rust-lang#123292

Also removes a bunch of extra diagnostics that were introduced in rust-lang#121154 and rust-lang#120558
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Apr 17, 2024
Silence some follow-up errors on trait impls in case the trait has conflicting or otherwise incoherent impls

fixes #123292

Also removes a bunch of extra diagnostics that were introduced in rust-lang/rust#121154 and rust-lang/rust#120558
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants