Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 10, 2025

Conversation

bricknerb
Copy link
Contributor

@bricknerb bricknerb commented Feb 6, 2025

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

Part of #4622.

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is probably fine, although the behavior should be expected to change when Core becomes a keyword.

// CHECK:STDERR: class Core {}
// CHECK:STDERR: ^~~~~~~~~~~~
// CHECK:STDERR:
class Core {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class Core {}
class r#Core {}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, suggesting this specifically so that making Core a keyword causes a corresponding behavior change in this test. Which I think is probably fine.

FYI @danakj

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

// --- fail_implicitly_poison_core.carbon
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this in testdata/function/declaration/...? It doesn't really seem to be testing function declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right, there a lot of tests here that are definitely not about function declaration and/or not specific to function declaration.
I thought of putting all name poisoning tests together, but I agree this is not a good place.
I think at least some of the tests don't really belong to any specific existing testdata directory.
Should I create a dedicated one? Perhaps "lookup"?
Will probably do this in a separate PR though if that's ok.

Copy link
Contributor

@jonmeow jonmeow Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4909 has testdata/packages/raw_core.carbon. You're testing name poisoning of the package; how about either that file or testdata/pavckages/core_name_poisoning.carbon?

Maybe this is small enough to just take care of in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved it to toolchain/check/testdata/packages/no_prelude/core_name_poisoning.carbon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I see you requested another review from zygoloid, but I think this is small enough to go ahead with a merge.

@bricknerb bricknerb requested a review from zygoloid February 7, 2025 09:19
…`toolchain/check/testdata/packages/no_prelude/core_name_poisoning.carbon`
@jonmeow jonmeow added this pull request to the merge queue Feb 10, 2025
Merged via the queue into carbon-language:trunk with commit e1c9cef Feb 10, 2025
8 checks passed
@bricknerb bricknerb deleted the poison2 branch February 13, 2025 23:24
bricknerb added a commit to bricknerb/carbon-lang that referenced this pull request Feb 20, 2025
…istent

This is also following carbon-language#4900 (comment), which points that name poisoning tests are not in the correct place.
github-merge-queue bot pushed a commit that referenced this pull request Feb 24, 2025
…istent (#4987)

This is also following
#4900 (comment),
which points that name poisoning tests are not in the correct place.
Part of #4622.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants