Bubble up post-monomorphization errors for the instantiation note#157318
Draft
xmakro wants to merge 2 commits into
Draft
Bubble up post-monomorphization errors for the instantiation note#157318xmakro wants to merge 2 commits into
xmakro wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
38bb530 to
e27e1f6
Compare
This comment has been minimized.
This comment has been minimized.
…count `collect_items_rec` snapshots the error count before processing a mono item and compares it afterwards to decide whether to attach the "the above error was encountered while instantiating ..." note. The mono item graph is walked in parallel, so the global error count can be bumped by an error emitted while collecting a different item on another thread between the two reads, which makes an unrelated item (often lang_start) get blamed. Instead of consulting the global count, bubble up an `ErrorGuaranteed` from the places that actually emit post-monomorphization errors during collection: const-eval failures in `eval_constant` and, through `check_mono_item`, the feature-dependent ABI checks and a `large_assignments` lint that has been escalated to `deny`/`forbid`. `items_of_instance` now returns whether such an error was encountered, and `collect_items_rec` uses that to decide whether to emit the note. This is unaffected by what other threads are doing. `large_assignments` is normally a warning and so does not hand back an `ErrorGuaranteed`. When it is escalated to an error we recover the proof token from the `DiagCtxt` right after emitting, gated on the lint's effective level at the relevant node so the note is only attributed to an instantiation whose own lint became the error.
These were marked ignore-parallel-frontend because the spurious instantiation note made their output nondeterministic under -Zthreads. With that fixed they pass reliably, so remove the directive and bless the line numbers that shift from removing it. Also add a regression test for a deny-level large_assignments lint inside a generic function, which must produce the instantiation note now that the lint is bubbled up as a post-monomorphization error.
e27e1f6 to
e1cade8
Compare
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This file contains hidden or 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
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.
Alternative implementation to #157282, opened so the two approaches can be compared. After implementing and testing it, the conclusion is that bubbling does not work for every post-monomorphization
error.
Bubbling requires every post-monomorphization error source to hand back an
ErrorGuaranteed. Some errors are emitted and then absorbed during collection, so no token reaches the collector along any return path.tests/ui/infinite/infinite-instantiation-struct-tail-ice-114484.rsis one such case, and it regresses under this PR (the three instantiation notes are dropped). Instrumenting the recursion-limit emit site instruct_tail_rawwith a backtrace shows that all twelve struct-tail errors in this test funnel through one path:The error is emitted inside CTFE while normalizing a callee
FnSigthat contains aPointee::Metadataprojection.struct_tail_rawcallsemit_errand returnsTy::new_error(reported);ptr_metadata_ty_or_tailthen maps that error tail to unit metadata (ty::Error(_) => Ok(tcx.types.unit)) and the token is dropped there. The const evaluation continues with unit metadata and returns a successfulOk, soeval_constanthas nothing to bubble. This is a single choke point, not a spread of sources, so the problem here is not that bubbling fails to scale to many emitters.Making the token survive the recovery does not work, and this was tried two ways. Returning
ty::Errormetadata for an error tail globally inptr_metadata_ty_or_tailturns the recovery into a runaway expansion: on thetest above the output grows from 13 errors and a clean abort to several million lines of struct-tail errors. Localizing the change so that only the trait-solver metadata projection keeps the
ty::Error(matching what the new solver does innormalizes_to), while the directptr_metadata_tycallers in codegen, CTFE and layout still recover to unit, does make the note fire again, but it still does not terminate: the collector then spirals through ever-deeperVirtualWrapper<.., N>instantiations, emitting the note hundreds of thousands of times without aborting. The unit-metadata recovery for an error tail is not just hiding the token; it is what makes these tailsSizedso the instantiation recursion can reach its limit and abort. It cannot be removed: this recovery is what avoids the original #114484 ICE (a rigidPointee::Metadataprojection left behind after the recursion limit), andptr_metadata_ty_or_tailis context-agnostic, so it cannot be made to fail only on the CTFE path without re-introducing that ICE in layout and codegen.