Skip to content
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

AstGen: disallow fields and decls from sharing names #21231

Merged
merged 6 commits into from
Aug 30, 2024

Conversation

mlugg
Copy link
Member

@mlugg mlugg commented Aug 28, 2024

This is a mini-proposal which is accepted as a prerequisite for #9938. The standard library and compiler needed a few changes to obey the new rules. Most of these are a net gain for readability, I'd say.


I discussed this change a little with @jacobly0 after running into some of the more awkward migrations in the compiler codebase. We reached the conclusion that, even ignoring #9938 entirely, this is a desirable language change. That's because certain namespace accesses today have some inherent ambiguity:

  • MyEnum.foo can refer to a declaration or an enum variant.
  • MyUnion.foo can also refer to a declaration or an enum variant (coercible to the union if the payload is void).
  • Even for structs, my_struct_val.foo() presents an ambiguity, since foo could be a function [pointer] field or a method. (This also applies to unions.)
  • N/A for opaques because they don't have fields.

Today, Zig makes arbitrary choices for how to resolve these ambiguities. The first two examples prefer decls, and the enum variant can be accessed with an enum literal (.foo). Conversely, method call syntax prefers a field if available, in which case the method can be accessed with MyStruct.foo(my_struct_val). These rules are awkward; they don't align with Zig's general perspective here of emitting a compile error upon ambiguity. So, the rule introduced by this PR, while occasionally slightly awkward, sidesteps this ambiguity issue by accepting that there are situations where field and declaration namespaces are conflated, so these namespaces should avoid conflicting with one another.

@mlugg mlugg added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. release notes This PR should be mentioned in the release notes. labels Aug 28, 2024
@mlugg mlugg force-pushed the field-decl-name-conflict branch 4 times, most recently from 662daa8 to 8f63a92 Compare August 28, 2024 23:59
@xdBronch
Copy link
Contributor

xdBronch commented Aug 29, 2024

if this change is only in astgen wont it still allow something like this?

const A = struct {
    foo: u8,
    usingnamespace B;
};
const B = struct {
    fn foo(_: A) void {}
};

is that intentional? is there an unofficial official consensus on usingnamespaces fate?

@mlugg
Copy link
Member Author

mlugg commented Aug 29, 2024

Agh, fuck, you're right, I forgot about usingnamespace.

I'm inclined to go ahead with this PR anyway, and solve the usingnamespace case down the line if #20663 is ultimately rejected. We definitely do want the AstGen check, we just might also want a check in Sema if usingnamespace sticks around.

lib/std/heap/sbrk_allocator.zig Outdated Show resolved Hide resolved
lib/std/pdb.zig Show resolved Hide resolved
@@ -110,12 +110,12 @@ pub const Instruction = struct {
offset: u64,
};

pub fn moffs(reg: Register, offset: u64) Memory {
pub fn initMoffs(reg: Register, offset: u64) Memory {
Copy link
Member

Choose a reason for hiding this comment

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

I think these "init" names are an unfortunate downgrade as a result of this breaking change, however, I have no suggestion to offer, and I think you found the best name given the new language constraints.

Related:

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, these were the ones that led me to discuss the change with Jacob. It's worth noting that these x86_64 ones were described by @jacobly0 as legacy, and their replacement doesn't have this problem.

This is a mini-proposal which is accepted as part of ziglang#9938.

This compiler and standard library need some changes to obey this rule.
Most of these changes seem like improvements. The PDB thing had a TODO
saying it used to crash; I anticipate it works now, we'll see what CI
does.

The `std.os.uefi` field renames are a notable breaking change.
@mlugg mlugg enabled auto-merge August 29, 2024 19:41
@mlugg mlugg force-pushed the field-decl-name-conflict branch 5 times, most recently from d41fdb9 to c3fb308 Compare August 29, 2024 22:43
These names aren't matching any formal specification; they're mostly
just ripped from LLVM code. Therefore, we should definitely follow Zig
naming conventions here.
Most of the required renames here are net wins for readaibility, I'd
say. The ones in `arch` are a little more verbose, but I think better. I
didn't bother renaming the non-conflicting functions in
`arch/arm/bits.zig` and `arch/aarch64/bits.zig`, since these backends
are pretty bit-rotted anyway AIUI.
@mlugg mlugg merged commit d997dda into ziglang:master Aug 30, 2024
10 checks passed
scheibo added a commit to pkmn/engine that referenced this pull request Aug 30, 2024
pleasantly surprised this is the ony thing that broke. very low
blast radius due to it being a private constant anyway.
baskuit pushed a commit to baskuit/engine that referenced this pull request Sep 1, 2024
pleasantly surprised this is the ony thing that broke. very low
blast radius due to it being a private constant anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. release notes This PR should be mentioned in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants