Change name poisoning implementation to allow better diagnostics #4764

bricknerb · 2025-01-07T13:10:42Z

Change the implementation to use an explicit is_poisoned bit instead of InstId::PoisonedName value.
Zero behavior change.
This would allow to more easily change the API to support accessing the poisoning declaration so we can have better name poisoning diagnosis.
#4622

…soned` bit instead of `InstId::PoisonedName` value. This would allow to more easily change the API to support accessing the poisoning delcaration so we can have better name poisoning diagnosis. carbon-language#4622

jonmeow

Generally LG, just some small comments that I think should be easy to fix and then merge.

jonmeow · 2025-01-07T18:36:02Z

toolchain/check/context.h

+  struct LookupNameInExactScopeResult {
+    SemIR::InstId inst_id;
+    SemIR::AccessKind access_kind;
+    bool is_poisoned = false;
+  };


Style thing, structs would belong at the top (https://google.github.io/styleguide/cppguide.html#Declaration_Order).

jonmeow · 2025-01-07T18:39:34Z

toolchain/check/context.h

  // Performs a name lookup in a specified scope, returning the referenced
  // instruction. Does not look into extended scopes. Returns an invalid
-  // instruction if the name is not found.
+  // instruction if the name is not found or poisoned.


This kind of sentence structure is difficult: does "not" apply to "found" only, or "found or poisoned"? Suggesting swapping order for clarity:

Suggested change

// instruction if the name is not found or poisoned.

// instruction if the name is poisoned or not found.

jonmeow · 2025-01-07T18:40:35Z

toolchain/check/context.h

+  struct LookupNameInExactScopeResult {
+    SemIR::InstId inst_id;
+    SemIR::AccessKind access_kind;
+    bool is_poisoned = false;


Maybe add comments to the members? Note this is sort of an alternative to the documentation on LookupNameInExactScope (which I note doesn't explain access_kind, but that's a pre-existing issue).

Suggested change

struct LookupNameInExactScopeResult {

SemIR::InstId inst_id;

SemIR::AccessKind access_kind;

bool is_poisoned = false;

struct LookupNameInExactScopeResult {

// The matching entity if found, or invalid if poisoned or not found.

SemIR::InstId inst_id;

// The access level required to use inst_id, if it's valid.

SemIR::AccessKind access_kind;

// Whether a poisoned entry was found.

bool is_poisoned = false;

Or if you'd rather keep documentation on the function, maybe just add a note of where to look:

Suggested change

struct LookupNameInExactScopeResult {

SemIR::InstId inst_id;

SemIR::AccessKind access_kind;

bool is_poisoned = false;

// See LookupNameInExactScope for return information.

struct LookupNameInExactScopeResult {

SemIR::InstId inst_id;

SemIR::AccessKind access_kind;

bool is_poisoned = false;

Added comments to the members, thanks!

jonmeow · 2025-01-07T18:40:50Z

toolchain/check/context.h

-  // declaration, returning the referenced instruction. If scope_id is invalid,
-  // uses the current contextual scope.
+  // declaration. If found, returns the referenced instruction and false. If
+  // poisoned, returns invalid instructions and true. poisoned. If scope_id is


Just a couple typos:

Suggested change

// poisoned, returns invalid instructions and true. poisoned. If scope_id is

// poisoned, returns an invalid instruction and true. If scope_id is

Might also want to rewrap the comment after the fix.

jonmeow · 2025-01-07T18:42:19Z

toolchain/check/context.h

+  // poisoned, returns invalid instructions and true. poisoned. If scope_id is
+  // invalid, uses the current contextual scope.


Maybe move the scope_id comment prior to the "If found", since it refers to the "specified scope" in the first sentence?

jonmeow · 2025-01-07T18:47:22Z

toolchain/check/decl_name_stack.h

-  // Adds a name to name lookup, or returns the existing instruction if this
+  // Adds a name to name lookup if not already declared or poisoned in this
+  // scope. If declared, returns the existing instruction and false. If
+  // poisoned, returns invalid instruction and true.


"returns invalid instruction" feels like it's missing "an", how about one of:

Suggested change

// poisoned, returns invalid instruction and true.

// poisoned, returns an invalid instruction and true.

Suggested change

// poisoned, returns invalid instruction and true.

// poisoned, returns invalid and true.

Added "an".
FWIW, I thought of "invalid instruction" as a term, but perhaps I should have used "InstId::Invalid" instead.

jonmeow · 2025-01-07T18:48:43Z

toolchain/check/decl_name_stack.h

+  // Adds a name to name lookup if not already declared or poisoned in this
+  // scope. If declared, returns the existing instruction and false. If
+  // poisoned, returns invalid instruction and true.
+  // TODO: Return the poisoning instruction if poisoned.
  // name has already been declared in this scope.


Looks like a typo:

Suggested change

// name has already been declared in this scope.

jonmeow · 2025-01-07T18:50:23Z

toolchain/check/decl_name_stack.h

@@ -237,10 +237,14 @@ class DeclNameStack {
  auto AddNameOrDiagnose(NameContext name_context, SemIR::InstId target_id,
                         SemIR::AccessKind access_kind) -> void;

-  // Adds a name to name lookup, or returns the existing instruction if this
+  // Adds a name to name lookup if not already declared or poisoned in this


This phrasing makes it hard to tell whether "not" applies to "poisoned". I think this doesn't overwrite poisoned entries, so how about using "neither ... nor" to emphasize:

Suggested change

// Adds a name to name lookup if not already declared or poisoned in this

// Adds a name to name lookup if neither already declared nor poisoned in this

If I'm wrong, swapping ordering could help:

Suggested change

// Adds a name to name lookup if not already declared or poisoned in this

// Adds a name to name lookup if poisoned or not already declared in this

Or, "either" can also help make it clear "not" wouldn't apply to "poisoned":

Suggested change

// Adds a name to name lookup if not already declared or poisoned in this

// Adds a name to name lookup if either not already declared or poisoned in this

jonmeow · 2025-01-07T19:01:10Z

toolchain/check/context.h

  SemIR::InstId inst_id;
+  // Whether the lookup found a poisoned name.
+  bool is_poisoned = false;


Is this default value used? I wonder if it might already be getting explicitly set, such that the default could be removed.

I think it depends on how the struct is initialized, so this is safer.
If in some function definition you just do
LookupResult result;
Then I think nothing guarantees is_poisoned would be false.

jonmeow · 2025-01-07T19:01:54Z

toolchain/check/context.h

+  struct LookupNameInExactScopeResult {
+    SemIR::InstId inst_id;
+    SemIR::AccessKind access_kind;
+    bool is_poisoned = false;


Is this default necessary, or could it be removed?

I think it depends on how the struct is initialized, so this is safer.
If in some function definition you just do
LookupNameInExactScopeResult result;
Then I think nothing guarantees is_poisoned would be false.

jonmeow · 2025-01-08T01:39:10Z

Small thing on the [NFC] tag -- we were talking about that in today's toolchain meeting, and it's an LLVM practice we'd prefer not to adopt (mainly because of the relationship with post-commit reviews in LLVM). Here I dropped it from the PR title; if you'd like to note it on a PR, just add something like "This change is only refactoring." That's the core thing I think you're trying to capture by adding it, but I think the description is also fine as-is.

Essentially, for now we're opting not to tag PRs and instead just rely on descriptions. That may change when we have more work going on, we'll wait and see. I'll update the code review docs tomorrow, but wanted to note here while the PR's pending. :)

github-actions bot added the toolchain label Jan 7, 2025

github-actions bot requested a review from josh11b January 7, 2025 13:10

bricknerb requested a review from jonmeow January 7, 2025 13:11

jonmeow approved these changes Jan 7, 2025

View reviewed changes

jonmeow changed the title ~~[NFC] Change name poisoning implementation to allow better diagnostics~~ Change name poisoning implementation to allow better diagnostics Jan 8, 2025

bricknerb added 2 commits January 8, 2025 08:16

Style and comments fixes from review.

30b5da5

Merge branch 'trunk' into poison2

a8f9122

bricknerb added this pull request to the merge queue Jan 8, 2025

Merged via the queue into carbon-language:trunk with commit 74395ce Jan 8, 2025
8 checks passed

bricknerb deleted the poison2 branch January 8, 2025 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change name poisoning implementation to allow better diagnostics #4764

Change name poisoning implementation to allow better diagnostics #4764

bricknerb commented Jan 7, 2025 •

edited

Loading

jonmeow left a comment

jonmeow Jan 7, 2025

bricknerb Jan 8, 2025

jonmeow Jan 7, 2025

bricknerb Jan 8, 2025

jonmeow Jan 7, 2025

bricknerb Jan 8, 2025

jonmeow Jan 7, 2025

bricknerb Jan 8, 2025

jonmeow Jan 7, 2025

bricknerb Jan 8, 2025

jonmeow Jan 7, 2025

bricknerb Jan 8, 2025

jonmeow Jan 7, 2025

bricknerb Jan 8, 2025

jonmeow Jan 7, 2025

bricknerb Jan 8, 2025

jonmeow Jan 7, 2025

bricknerb Jan 8, 2025

jonmeow Jan 7, 2025

bricknerb Jan 8, 2025

jonmeow commented Jan 8, 2025

	// instruction if the name is not found or poisoned.
	// instruction if the name is poisoned or not found.

	// poisoned, returns invalid instructions and true. poisoned. If scope_id is
	// poisoned, returns an invalid instruction and true. If scope_id is

		// poisoned, returns invalid instructions and true. poisoned. If scope_id is
		// invalid, uses the current contextual scope.

	// poisoned, returns invalid instruction and true.
	// poisoned, returns invalid and true.

	// Adds a name to name lookup if not already declared or poisoned in this
	// Adds a name to name lookup if neither already declared nor poisoned in this

	// Adds a name to name lookup if not already declared or poisoned in this
	// Adds a name to name lookup if poisoned or not already declared in this

	// Adds a name to name lookup if not already declared or poisoned in this
	// Adds a name to name lookup if either not already declared or poisoned in this

Change name poisoning implementation to allow better diagnostics #4764

Change name poisoning implementation to allow better diagnostics #4764

Conversation

bricknerb commented Jan 7, 2025 • edited Loading

jonmeow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow commented Jan 8, 2025

bricknerb commented Jan 7, 2025 •

edited

Loading