-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Add enum variants to the type namespace #17338
Conversation
Boolean(bool), | ||
List(List), | ||
Object(Object), | ||
Array(List), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be List(JsonList)
instead of Array(List)
? This is more consistent with Object(JsonObject)
, and causes less breakage since the variant name is used (for matching) probably more often the type name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json.org actually calls it "array" so this renaming I think is good, but perhaps json::List
could be changed to json::JsonArray
as well, for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shrugs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I fixed this as List{JsonList) to be consistent with Object. I thought changing to Array(JsonArray) was a bigger change and overstepped the mark of this PR.
r=me |
5af123d
to
392b33d
Compare
@bors retry |
This basically looks good to me, but it'd be helpful to separate into 3 commits:
It seems we ought to vet the last items carefully. Also, of course some compile-fail tests. |
Oh, I see @brson already gave r+, github was hiding it from me :( |
Anyway, the public-facing names I looked at all seemed pretty reasonable. |
@nick29581 oh, one thing, can you add a test for what happens when you use a variant as a type now? I didn't see that case covered... |
@bors: retry |
Change to resolve and update compiler and libs for uses. [breaking-change] Enum variants are now in both the value and type namespaces. This means that if you have a variant with the same name as a type in scope in a module, you will get a name clash and thus an error. The solution is to either rename the type or the variant.
392b33d
to
ce0907e
Compare
fix: do not resolve prelude within block modules fix rust-lang#17338 (continuing from rust-lang#17251). In rust-lang#17251, we injected preludes into non-top-level modules, which leading to r-a to directly resolve names in preludes in block modules. This PR fix it by checking whether the module is a pseudo-module introduced by blocks. (similar to what we do for extern preludes)
Change to resolve and update compiler and libs for uses.
[breaking-change]
Enum variants are now in both the value and type namespaces. This means that
if you have a variant with the same name as a type in scope in a module, you
will get a name clash and thus an error. The solution is to either rename the
type or the variant.
closes #17323
r? anyone? (I realise this still needs a cfail test (and the commit message, I just noticed), but otherwise does it look OK? I expect some of the new names could be better).