Repository: carbon-language/carbon-lang
File: toolchain/check/check_unit.cpp (C++)

Code before:
                                    /*poison_concrete_results=*/false);
    CARBON_CHECK(witness_result.has_concrete_value());
    auto witness_id = witness_result.concrete_witness();
    if (witness_id != poison.impl_witness) {
      // We can get the `Impl` from the resulting witness here, which is the
      // `Impl` that conflicts with the previous poison query.
      auto table_id = context_.insts()
                          .GetAs<SemIR::ImplWitness>(witness_id)
                          .witness_table_id;
      auto impl_id =
          context_.insts().GetAs<SemIR::ImplWitnessTable>(table_id).impl_id;
      const auto& impl = context_.impls().Get(impl_id);
      auto interface_name_id =
          context_.interfaces().Get(impl.interface.interface_id).name_id;
      CARBON_DIAGNOSTIC(
          PoisonedImplLookupConcreteResult, Error,
          "found `impl` that changes the result of a previous use "
          "of interface `{0}`",
          SemIR::NameId);
      auto builder =
          emitter_.Build(impl.first_decl_id(), PoisonedImplLookupConcreteResult,
                         interface_name_id);
      CARBON_DIAGNOSTIC(PoisonedImplLookupConcreteResultNote, Note,
                        "use of interface `{0}` here", SemIR::NameId);
      builder.Note(poison.loc_id, PoisonedImplLookupConcreteResultNote,
                   interface_name_id);
      builder.Emit();
    }
  }
  context_.inst_block_stack().PopAndDiscard();
}

auto CheckUnit::FinishRun() -> void {
  CheckRequiredDeclarations();
  CheckRequiredDefinitions();
  CheckPoisonedConcreteImplLookupQueries();

  // Pop information for the file-level scope.
  context_.sem_ir().set_top_inst_block_id(context_.inst_block_stack().Pop());
  context_.scope_stack().Pop();

  // Finalizes the list of exports on the IR.
  context_.inst_blocks().ReplacePlaceholder(SemIR::InstBlockId::Exports,
                                            context_.exports());
  // Finalizes the ImportRef inst block.
  context_.inst_blocks().ReplacePlaceholder(SemIR::InstBlockId::ImportRefs,
                                            context_.import_ref_ids());
  // Finalizes __global_init.
  context_.global_init().Finalize();

  context_.sem_ir().set_has_errors(unit_and_imports_->err_tracker.seen_error());

Review comment (suggestion):
I think it'd be good to list the full self type and interface type from the query here. Perhaps something like:
```suggestion
      CARBON_DIAGNOSTIC(
          PoisonedImplLookupConcreteResult, Error,
          "found `impl` that changes the result of a previous use "
          "of `{0} as {1}`",
         InstIdAsType, InstIdAsType);
      auto builder =
          emitter_.Build(impl.first_decl_id(), PoisonedImplLookupConcreteResult,
                         query.query_self_inst_id, query.query_specific_interface_id);
      CARBON_DIAGNOSTIC(PoisonedImplLookupConcreteResultNote, Note,
                        "impl used here");
      builder.Note(poison.loc_id, PoisonedImplLookupConcreteResultNote);
```
(And I think it's better to not repeat the same information in the note.)

Code after:
                                    poison.non_canonical_query_self_inst_id,
                                    /*poison_concrete_results=*/false);
    CARBON_CHECK(witness_result.has_concrete_value());
    auto found_witness_id = witness_result.concrete_witness();
    if (found_witness_id != poison.impl_witness) {
      auto witness_to_impl_id = [&](SemIR::InstId witness_id) {
        auto table_id = context_.insts()
                            .GetAs<SemIR::ImplWitness>(witness_id)
                            .witness_table_id;
        return context_.insts()
            .GetAs<SemIR::ImplWitnessTable>(table_id)
            .impl_id;
      };

      // We can get the `Impl` from the resulting witness here, which is the
      // `Impl` that conflicts with the previous poison query.
      auto bad_impl_id = witness_to_impl_id(found_witness_id);
      const auto& bad_impl = context_.impls().Get(bad_impl_id);

      auto prev_impl_id = witness_to_impl_id(poison.impl_witness);
      const auto& prev_impl = context_.impls().Get(prev_impl_id);

      CARBON_DIAGNOSTIC(
          PoisonedImplLookupConcreteResult, Error,
          "found `impl` that would change the result of an earlier "
          "use of `{0} as {1}`",
          InstIdAsRawType, SpecificInterfaceIdAsRawType);
      auto builder =
          emitter_.Build(poison.loc_id, PoisonedImplLookupConcreteResult,
                         poison.query.query_self_inst_id,
                         poison.query.query_specific_interface_id);
      CARBON_DIAGNOSTIC(
          PoisonedImplLookupConcreteResultNoteBadImpl, Note,
          "the use would select the `impl` here but it was not found yet");
      builder.Note(bad_impl.first_decl_id(),
                   PoisonedImplLookupConcreteResultNoteBadImpl);
      CARBON_DIAGNOSTIC(PoisonedImplLookupConcreteResultNotePreviousImpl, Note,
                        "the use had selected the `impl` here");
      builder.Note(prev_impl.first_decl_id(),
                   PoisonedImplLookupConcreteResultNotePreviousImpl);
      builder.Emit();
    }
  }
  context_.inst_block_stack().PopAndDiscard();
}

auto CheckUnit::FinishRun() -> void {
  CheckRequiredDeclarations();
  CheckRequiredDefinitions();
  CheckPoisonedConcreteImplLookupQueries();