search

carbon-language / carbon-lang

Carbon Language's main repository: documents, design, implementation, and related tools. (NOTE: Carbon Language is experimental; see README)
http://docs.carbon-lang.dev/
Other
32.24k stars 1.48k forks source link

Address the LocIdAndInst::ReusingLoc TODO #4211

Closed jonmeow closed 1 month ago

jonmeow commented 1 month ago

The TODO for switching to ReusingLoc (previously Untyped) had been there for a while, so I'm trying to address it here. The intent had been to be clearer about when the construction is validated, particularly so that we aren't accidentally accepting an incorrect NodeId. Note, this does fix an incorrect use of InvalidNodeId where NoLoc should've been called.

Since this changes the semantics of when Parse::NodeId is helpful in typed_nodes.h, I'm doing a pass to either refine or switch to Parse::InvalidNodeId where it compiles. I think most remaining Parse::NodeId examples are things we should be able to refine with a little more work (versus before where Parse::NodeId also indicated LocId construction might be used).

I'm also changing context.h to use requires that match what LocIdAndInst has, I think it makes the diagnostics a little better. And note I do add an overload for ImportIRInstId, also matching LocIdAndInst, and widely used for import refs.

jonmeow commented 1 month ago

Note an alternative here is to just stop validating so much, removing ReusingLoc and just provide the LocId constructor.

I'm hoping @zygoloid has time to review before vanishing, but @josh11b I thought you might make a good backup since you added a lot of the NodeId restrictions.

chandlerc commented 1 month ago

(I'm not merging because I can't tell whether @zygoloid's concerns have been addressed)

FWIW, while I'm not sure they have either, I do think they've been captured in a TODO and we should be able to revisit this in the future. I don't think we need to block this PR, especially with vacation.

geoffromer commented 1 month ago

I don't think we need to block this PR, especially with vacation.

Just to be clear, I didn't intend to block this (hence my approval), I just intended to let @jonmeow make the decision about whether it was ready to merge.