Refactor name poisoning tests to be more organized, complete and consistent #4987
Conversation
…istent This is also following carbon-language#4900 (comment), which points that name poisoning tests are not in the correct place.
| // EXTRA-ARGS: --no-dump-sem-ir | ||
| // |
There was a problem hiding this comment.
The sem-ir dump shows what symbols are poisoned so it's useful to verify the behavior or reason about it.
There was a problem hiding this comment.
Okay, just know that the sem-ir dumps are quite long and it is easy to miss information in there.
| // N.F uses N.C and not C. | ||
| namespace N; | ||
| class N.C; | ||
| fn N.F(x: C); |
There was a problem hiding this comment.
Seems like there should be a call that verifies the truth of the comment, like:
fn TestCall(y: N.C) {
// `N.F` accepts an `N.C` not a `package.C`.
N.F(y);
}
There was a problem hiding this comment.
Done.
Note though that this is implicitly tested in the sem ir dump.
There was a problem hiding this comment.
It is much better to have a test that will fail if something goes wrong than a change that needs to be manually reviewed to see that it is broken.
toolchain/check/testdata/class/no_prelude/name_poisoning.carbon
Outdated
Show resolved
Hide resolved
toolchain/check/testdata/function/declaration/no_prelude/name_poisoning.carbon
Outdated
Show resolved
Hide resolved
toolchain/check/testdata/function/declaration/no_prelude/name_poisoning.carbon
Outdated
Show resolved
Hide resolved
toolchain/check/testdata/function/declaration/no_prelude/name_poisoning.carbon
Show resolved
Hide resolved
| // N.F uses N.I and not I. | ||
| namespace N; | ||
| interface N.I {} | ||
| fn N.F(x: I); |
There was a problem hiding this comment.
| fn N.F(x: I); | |
| fn N.F(x:! I); |
There was a problem hiding this comment.
Can you clarify why does using generics help here?
I've tried this and the below and I get generics related errors.
There was a problem hiding this comment.
The values of an interface are conceptually compile-time values. x: I would almost certainly be a mistake in user code and is something we might mark as an error in the future. What error do you get?
toolchain/check/testdata/namespace/no_prelude/name_poisoning.carbon
Outdated
Show resolved
Hide resolved
| // EXTRA-ARGS: --no-dump-sem-ir | ||
| // |
There was a problem hiding this comment.
Okay, just know that the sem-ir dumps are quite long and it is easy to miss information in there.
| // N.F uses N.C and not C. | ||
| namespace N; | ||
| class N.C; | ||
| fn N.F(x: C); |
There was a problem hiding this comment.
It is much better to have a test that will fail if something goes wrong than a change that needs to be manually reviewed to see that it is broken.
| // N.F uses N.I and not I. | ||
| namespace N; | ||
| interface N.I {} | ||
| fn N.F(x: I); |
There was a problem hiding this comment.
The values of an interface are conceptually compile-time values. x: I would almost certainly be a mistake in user code and is something we might mark as an error in the future. What error do you get?
| namespace N2; | ||
| namespace N2.N1; | ||
| alias N2.N3 = N1; | ||
|
|
||
| class N2.N1.C {} |
There was a problem hiding this comment.
| class N2.N1.C {} | |
| class N2.N1.C {} | |
| class N1.C {} |
This is also following #4900 (comment), which points that name poisoning tests are not in the correct place.
Part of #4622.