When poisoning a name, keep the triggering instruction instead of the triggering location

[bricknerb]

This discussion: https://discord.com/channels/655572317891461132/655578254970716160/1349053005609046108

Requires using AddPlaceholderInst() and ReplaceInstBeforeConstantUse() instead of AddInst() in some cases to allow using the InstId when looking up the name.

Destroy is a special case (in LookupMemberNameInScope()) because we only add it if lookup is fails finding the name.

Part of #4622.

[jonmeow]

Two things here:

1. If you have specific examples for how this would improve diagnostics, can you please give details (with specific examples)? Bear in mind we want to use LocId for diagnostics.

2. This code seems to be changing the generated IR by adding instructions, which I wouldn't expect -- changing an instruction to use placeholders shouldn't affect that. I'm not looking closely, but to be sure, I would suspect a bug.

[bricknerb]

Two things here:

1. If you have specific examples for how this would improve diagnostics, can you please give details (with specific examples)? Bear in mind we want to use LocId for diagnostics.
2. This code seems to be changing the generated IR by adding instructions, which I wouldn't expect -- changing an instruction to use placeholders shouldn't affect that. I'm not looking closely, but to be sure, I would suspect a bug.

Thanks!

1. I've improved the description to provide better context.
2. Couldn't find something suspicious in the code quickly, will debug further.

[jonmeow]

1. I've improved the description to provide better context.

Thanks! The diagnostic structure has changed since two months ago. We now accept an InstId inside a LocId so it's not obvious to me from that link that this needs to be done. Maybe it'd make sense to close this PR if that's the only reason you're doing it?

[bricknerb]

1. I've improved the description to provide better context.

Thanks! The diagnostic structure has changed since two months ago. We now accept an InstId inside a LocId so it's not obvious to me from that link that this needs to be done. Maybe it'd make sense to close this PR if that's the only reason you're doing it?

Makes sense.

Another potential reason is for importing poisoned names from api to impl.
Context:

carbon-lang/toolchain/check/testdata/function/declaration/no_prelude/name_poisoning.carbon

Line 174 in 5226f3d

// TODO: #4622 This should fail since `N.F1` was poisoned in the api.

I considered doing that by adding a poisoned name in impl, with a poisoning location pointing to the api poisoning location. The LocId that can point to another IR is ImportIRInstId, and to create ImportIRInst we need InstId.
Does my logic makes sense?

Alternatively, we can add a new LocId::Kind that supports pointing to another IR with a LocId.

WDYT?

[jonmeow]

Makes sense.

Another potential reason is for importing poisoned names from api to impl. Context:

carbon-lang/toolchain/check/testdata/function/declaration/no_prelude/name_poisoning.carbon

Line 174 in 5226f3d

// TODO: #4622 This should fail since `N.F1` was poisoned in the api.

I considered doing that by adding a poisoned name in impl, with a poisoning location pointing to the api poisoning location. > The LocId that can point to another IR is ImportIRInstId, and to create ImportIRInst we need InstId. Does my logic makes sense?

Sorry, I wasn't seeing part of your plan here. This additional detail helps, but mostly I think I can offer clearer advice. I still think you should work this through to the point that the diagnostic changes you want are implemented, either in this PR or a stacked PR. For example here, you mention switching the poisoning location to an ImportIRInstId.

Where would the poisoning ImportIRInstId stored? If the storage is a LocId, LocId to store both an InstId and ImportIRInstId; reading between the lines, all you really want is for it to not be a NodeId. If the storage is an InstId, obviously you need different storage for an ImportIRInstId.

Also, to note the regressions in this PR, the golden test output should really stay the same in general. I think that fixing those bugs will also require altering your approach a little. For example, I do see some changes to order of instruction creation which look flawed (e.g., swapping ordering in handle_name.cpp of the instructions there); that's a temporal bug, instructions can only use previous instructions as inputs. There are probably a couple different ways to handle that, e.g. using a callback to construct the poisoning instruction rather than requiring the InstId as input (or just returning a handle that allows changing the poisoning location later).

Alternatively, we can add a new LocId::Kind that supports pointing to another IR with a LocId.

LocId is already a complex structure, and my instinct here is there are simpler solutions here. If this option is looking appealing to you, my suggestion would be -- because diagnostic quality is not a priority right now, and we're trying to just get in core functionality -- it would be better to set this change aside and focus on higher priority work.