When looking up `Core`, pass `is_being_declared` as `true` #4903

bricknerb · 2025-02-06T15:57:20Z

There's no need to try and poison Core.
This doesn't change tests since Core isn't being poisoned anyways.
Part of #4622.

danakj · 2025-02-06T16:05:12Z

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

@@ -369,6 +369,16 @@ fn F() {
  }
 }

+// --- fail_dont_implicitly_poison_core.carbon
+
+// CHECK:STDERR: fail_dont_implicitly_poison_core.carbon:[[@LINE+4]]:9: error: `Core.Bool` implicitly referenced here, but package `Core` not found [CoreNotFound]


I find this error to be pretty confusing. I would easily try to fix it by trying to understand why Core is not found - is my installation messed up? Where is Core? But of course the answer is I named something Core later in the file.

That said, I think the way we want to resolve this is via https://github.com/carbon-language/carbon-lang/pull/4864/files, which is to make Core a keyword. So the class Core would not in fact make a class called Core, it would be an invalid use of the Core keyword. Then we should not even have an error here, right?

AFAIU, actually, the reason Core is not found is because this is a no_prelude test so Core is not loaded.
It's not found similar to other no_prelude tests.
This test is different from other tests that trigger this error because it shows that this doesn't cause Core to be poisoned, unlike #4900.

If Core is a keyword, we would still have this error.
We might have another error saying that class Core cannot be defined because Core is a keyword.

Oh okay, thanks. I misunderstood the relationship to #4900.

The failure looks good then, but worrying about poisoning Core specifically is a transient concern, since we won't be able to write class Core once we make it a keyword. At that point would we just remove this test? Do you think it's worth adding still as-is?

Changed to class r#Core.

danakj · 2025-02-06T16:20:47Z

toolchain/check/context.cpp

@@ -703,7 +703,7 @@ static auto GetCorePackage(Context& context, SemIR::LocId loc_id,
  // Look up `package.Core`.
  auto core_scope_result = context.LookupNameInExactScope(
      loc_id, core_name_id, SemIR::NameScopeId::Package,
-      context.name_scopes().Get(SemIR::NameScopeId::Package));
+      context.name_scopes().Get(SemIR::NameScopeId::Package), true);


I do think this is the right change to make - not sure how the keyword impl shakes out but we don't want to poison things.

Suggested change

context.name_scopes().Get(SemIR::NameScopeId::Package), true);

context.name_scopes().Get(SemIR::NameScopeId::Package), /*is_being_declared=*/true);

But I am not sure how to test this in a way that would survive the keyword change, and welcome thoughts on that.

Done (and changed to r#Core).

danakj · 2025-02-06T16:23:03Z

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

+// CHECK:STDERR:
+fn F(x: bool);
+
+class Core {}


Suggested change

class Core {}

class r#Core {}

Would this test the same thing now - and after the keyword change?

I believe so, done.

jonmeow · 2025-02-06T18:31:01Z

There's an open proposal to make Core a keyword. I think #4900 is more correct.

zygoloid

Perhaps we should stop using name lookup to find the Core package. We could instead track the Core package specially on the SemIR::File.

bricknerb · 2025-02-07T09:30:09Z

Perhaps we should stop using name lookup to find the Core package. We could instead track the Core package specially on the SemIR::File.

This makes sense though I assume there might be complexity cost for special casing Core.

danakj · 2025-02-07T14:51:51Z

There's an open proposal to make Core a keyword. I think #4900 is more correct.

Yeah, I can't tell which of the two PRs is more correct myself, but maybe we should rebase either one on #4909 at which point r#Core won't be poisoned, so the test in here should pass without any code changes?

Alternatively, if poisoning `Core` is considered a bug, we could avoid poisoning it (see #4903). Part of #4622.

danakj

LGTM

bricknerb · 2025-02-13T23:02:40Z

I think other changes made this change different, so I'm moving this to draft.

…ere's no need to try and poison `Core`. This doesn't change tests since `Core` isn't being poisoned anyways. Part of carbon-language#4622.

bricknerb · 2025-02-14T00:23:26Z

Sorry for the force push, I had to redo this change given recent changes.

jonmeow · 2025-02-14T00:30:11Z

@bricknerb Given #4884, isn't this change a no-op? I ask because also, isn't setting "is_being_declared" when we're doing a non-declaring name lookup a little confusing for future readers?

bricknerb · 2025-02-14T00:41:13Z

@bricknerb Given #4884, isn't this change a no-op?

Yes!

I ask because also, isn't setting "is_being_declared" when we're doing a non-declaring name lookup a little confusing for future readers?

The name is_being_declared might be misleading. It's currently set to true only when called from LookupNameInDecl() in a scope. In other words, when the name being looked up is known to be in a specific scope.
That's the case here, we know the scope of Core, so it makes more sense to pass true.
WDYT?

jonmeow · 2025-02-14T16:26:02Z

@bricknerb Given #4884, isn't this change a no-op?

Yes!

I ask because also, isn't setting "is_being_declared" when we're doing a non-declaring name lookup a little confusing for future readers?

The name is_being_declared might be misleading. It's currently set to true only when called from LookupNameInDecl() in a scope. In other words, when the name being looked up is known to be in a specific scope. That's the case here, we know the scope of Core, so it makes more sense to pass true. WDYT?

I think that Core is a top-level name, and should pretty much always be there (even before #4884, poisoning would have rarely run and could have poisoned at most one namespace). This is not declaring it, so it will only confuse readers to set "is_being_declared". This PR should be closed, not merged (to be clear, this is what I also meant when I said #4900 is more correct).

jonmeow · 2025-02-14T17:02:31Z

BTW, LookupNameInDecl sets is_being_declared to true because it's part of declaring names. I think the name is correct.

danakj · 2025-02-14T17:06:23Z

@bricknerb Given #4884, isn't this change a no-op?

Yes!

I ask because also, isn't setting "is_being_declared" when we're doing a non-declaring name lookup a little confusing for future readers?

The name is_being_declared might be misleading. It's currently set to true only when called from LookupNameInDecl() in a scope. In other words, when the name being looked up is known to be in a specific scope. That's the case here, we know the scope of Core, so it makes more sense to pass true. WDYT?

I think that Core is a top-level name, and should pretty much always be there (even before #4884, poisoning would have rarely run and could have poisoned at most one namespace). This is not declaring it, so it will only confuse readers to set "is_being_declared". This PR should be closed, not merged (to be clear, this is what I also meant when I said #4900 is more correct).

Hm yeah, when I reviewed earlier it was just a test showing that using Core implicitly did not prevent r#Core from being used. Verifying r#Core works is good. But I agree we don't need to pass is_being_declared for Core - it's not declaring it, it's a keyword.

bricknerb · 2025-02-14T17:30:54Z

BTW, LookupNameInDecl sets is_being_declared to true because it's part of declaring names. I think the name is correct.

Sure, in LookupNameInDecl() context it's correct, just not sure that it would stay correct in LookupNameInExactScope() context for any (future?) call, given how it's used.
I guess declaring and poisoning together does make sense in principal.

bricknerb · 2025-02-14T17:31:29Z

@bricknerb Given #4884, isn't this change a no-op?

Yes!

I ask because also, isn't setting "is_being_declared" when we're doing a non-declaring name lookup a little confusing for future readers?

The name is_being_declared might be misleading. It's currently set to true only when called from LookupNameInDecl() in a scope. In other words, when the name being looked up is known to be in a specific scope. That's the case here, we know the scope of Core, so it makes more sense to pass true. WDYT?

I think that Core is a top-level name, and should pretty much always be there (even before #4884, poisoning would have rarely run and could have poisoned at most one namespace). This is not declaring it, so it will only confuse readers to set "is_being_declared". This PR should be closed, not merged (to be clear, this is what I also meant when I said #4900 is more correct).

SG, closing.

bricknerb requested a review from jonmeow February 6, 2025 15:57

github-actions bot requested a review from danakj February 6, 2025 15:57

github-actions bot added the toolchain label Feb 6, 2025

bricknerb mentioned this pull request Feb 6, 2025

Add a test showing how Core can be implicitly poisoned #4900

Merged

danakj reviewed Feb 6, 2025

View reviewed changes

zygoloid reviewed Feb 6, 2025

View reviewed changes

bricknerb requested review from zygoloid and danakj February 7, 2025 09:30

github-merge-queue bot pushed a commit that referenced this pull request Feb 10, 2025

Add a test showing how Core can be implicitly poisoned (#4900)

e1c9cef

Alternatively, if poisoning `Core` is considered a bug, we could avoid poisoning it (see #4903). Part of #4622.

danakj approved these changes Feb 13, 2025

View reviewed changes

bricknerb marked this pull request as draft February 13, 2025 23:02

When looking up Core, pass is_being_declared as true because th…

e8c294e

…ere's no need to try and poison `Core`. This doesn't change tests since `Core` isn't being poisoned anyways. Part of carbon-language#4622.

bricknerb force-pushed the poison4 branch from c24c46d to e8c294e Compare February 14, 2025 00:19

bricknerb changed the title ~~Avoid implicitly poisoning Core~~ When looking up Core, pass is_being_declared as true Feb 14, 2025

bricknerb marked this pull request as ready for review February 14, 2025 00:22

github-actions bot requested a review from chandlerc February 14, 2025 00:23

bricknerb requested a review from danakj February 14, 2025 00:23

bricknerb closed this Feb 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When looking up `Core`, pass `is_being_declared` as `true` #4903

When looking up `Core`, pass `is_being_declared` as `true` #4903

bricknerb commented Feb 6, 2025 •

edited

Loading

danakj Feb 6, 2025

bricknerb Feb 6, 2025

danakj Feb 6, 2025

bricknerb Feb 7, 2025

danakj Feb 6, 2025

bricknerb Feb 7, 2025

danakj Feb 6, 2025

bricknerb Feb 7, 2025

jonmeow commented Feb 6, 2025

zygoloid left a comment

bricknerb commented Feb 7, 2025

danakj commented Feb 7, 2025

danakj left a comment

bricknerb commented Feb 13, 2025

bricknerb commented Feb 14, 2025

jonmeow commented Feb 14, 2025

bricknerb commented Feb 14, 2025 •

edited

Loading

jonmeow commented Feb 14, 2025 •

edited

Loading

jonmeow commented Feb 14, 2025

danakj commented Feb 14, 2025

bricknerb commented Feb 14, 2025 •

edited

Loading

bricknerb commented Feb 14, 2025

	context.name_scopes().Get(SemIR::NameScopeId::Package), true);
	context.name_scopes().Get(SemIR::NameScopeId::Package), /is_being_declared=/true);

When looking up Core, pass is_being_declared as true #4903

When looking up Core, pass is_being_declared as true #4903

Conversation

bricknerb commented Feb 6, 2025 • edited Loading

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 Feb 6, 2025

zygoloid left a comment

Choose a reason for hiding this comment

bricknerb commented Feb 7, 2025

danakj commented Feb 7, 2025

danakj left a comment

Choose a reason for hiding this comment

bricknerb commented Feb 13, 2025

bricknerb commented Feb 14, 2025

jonmeow commented Feb 14, 2025

bricknerb commented Feb 14, 2025 • edited Loading

jonmeow commented Feb 14, 2025 • edited Loading

jonmeow commented Feb 14, 2025

danakj commented Feb 14, 2025

bricknerb commented Feb 14, 2025 • edited Loading

bricknerb commented Feb 14, 2025

When looking up `Core`, pass `is_being_declared` as `true` #4903

When looking up `Core`, pass `is_being_declared` as `true` #4903

bricknerb commented Feb 6, 2025 •

edited

Loading

bricknerb commented Feb 14, 2025 •

edited

Loading

jonmeow commented Feb 14, 2025 •

edited

Loading

bricknerb commented Feb 14, 2025 •

edited

Loading