Parser: improve typeof support #27

ehaas · 2021-09-13T17:30:48Z

Add two new type specifiers: typeof_type and typeof_expr, which
are the types returned by typeof (depending on whether it's called
with a type or an expression)

This allows us to track the underlying type or expression that was
used.

ehaas · 2021-09-13T17:34:53Z

Curious what you think of this approach.

My biggest concern is that the various typeof_ types could be a bit of a footgun if they're not explicitly checked any time types are dealt with.

There are a bunch of places in Parser where a type specifier is directly checked for equality; these would all need to be updated to use something like ty.is(specifier) in order to check inside the typeof types.

Vexu

Curious what you think of this approach.

My biggest concern is that the various typeof_ types could be a bit of a footgun if they're not explicitly checked any time types are dealt with.

There are a bunch of places in Parser where a type specifier is directly checked for equality; these would all need to be updated to use something like ty.is(specifier) in order to check inside the typeof types.

Those places would've needed to be updated sooner or later anyways for attributed types. That is unless we find some other, better way to represent them.

Vexu · 2021-09-15T14:21:33Z

src/Parser.zig

    }
    const typeof_expr = try p.parseNoEval(expr);
    try typeof_expr.expect(p);
    try p.expectClosing(l_paren, .r_paren);
-    return typeof_expr.ty;
+
+    const inner = try p.arena.create(Type.VLA);


It would probably be appropriate to rename Type.VLA to Type.Expr or something.

Renamed it to Type.Expr and renamed the members to be (I think) more consistent. It's in its own commit so should be easy to revert or change if you don't like it.

Vexu · 2021-09-15T14:24:23Z

src/Type.zig

+            try w.writeAll(")");
+        },
+        .typeof_expr, .decayed_typeof_expr => {
+            try w.writeAll("typeof(<expr>: ");


Someday I'll go through the effort of adding a token field to Node and then this can be changed to pretty print the expression like clang.

Vexu

I propose adding a fn get(ty: *const Type, specifier: Specifier) ?*const Type function to make correct usage easier.

Vexu · 2021-09-16T06:08:35Z

src/Parser.zig

@@ -1909,7 +1933,7 @@ fn initializerItem(p: *Parser, il: *InitList, init_ty: Type) Error!bool {
                    } else @intCast(u64, val),
                    .unavailable => unreachable,
                };
-                const max_len = if (cur_ty.specifier == .array) cur_ty.data.array.len else std.math.maxInt(usize);
+                const max_len = if (cur_ty.is(.array)) cur_ty.data.array.len else std.math.maxInt(usize);


This will lead to invalid usage of cur_ty.data, adding an arrayLen would stop the repetition of this line.

Vexu · 2021-09-16T06:09:54Z

src/Parser.zig

@@ -2018,7 +2042,7 @@ fn findScalarInitializer(p: *Parser, il: **InitList, ty: *Type) Error!bool {
            if (try p.findScalarInitializer(il, ty)) return true;
        }
        return false;
-    } else if (ty.specifier == .@"struct") {
+    } else if (ty.is(.@"struct")) {


This is also invalid, the function uses the fields on ty extensively so there should be an unwrap at the top.

Vexu · 2021-09-16T17:13:34Z

src/Parser.zig

@@ -959,7 +983,7 @@ fn initDeclarator(p: *Parser, decl_spec: *DeclSpec) Error!?InitDeclarator {

        var init_list_expr = try p.initializer(init_d.d.ty);
        init_d.initializer = init_list_expr.node;
-        if (init_d.d.ty.specifier == .incomplete_array) {
+        if (init_d.d.ty.is(.incomplete_array)) {


This will lead to invalid usage of ty.data.

Vexu · 2021-09-16T17:21:18Z

src/Parser.zig

@@ -2039,7 +2063,7 @@ fn findScalarInitializer(p: *Parser, il: **InitList, ty: *Type) Error!bool {
            if (try p.findScalarInitializer(il, ty)) return true;
        }
        return false;
-    } else if (ty.specifier == .@"union") {
+    } else if (ty.is(.@"union")) {


Also invalid.

Vexu · 2021-09-16T17:24:16Z

src/Parser.zig

@@ -2203,7 +2227,7 @@ fn convertInitList(p: *Parser, il: InitList, init_ty: Type) Error!NodeIndex {
            .data = .{ .bin = .{ .lhs = .none, .rhs = .none } },
        };

-        if (init_ty.specifier == .incomplete_array) {
+        if (init_ty.is(.incomplete_array)) {


arr_init_node == init_ty so the below assignments should happen on a duped type (at least if init_ty is not a meta type).

Vexu · 2021-09-16T17:24:32Z

src/Parser.zig

@@ -2226,7 +2250,7 @@ fn convertInitList(p: *Parser, il: InitList, init_ty: Type) Error!NodeIndex {
            },
        }
        return try p.addNode(arr_init_node);
-    } else if (init_ty.specifier == .@"struct") {
+    } else if (init_ty.is(.@"struct")) {


Will also lead to invalid use.

Vexu · 2021-09-16T17:24:46Z

src/Parser.zig

@@ -2258,7 +2282,7 @@ fn convertInitList(p: *Parser, il: InitList, init_ty: Type) Error!NodeIndex {
            },
        }
        return try p.addNode(struct_init_node);
-    } else if (init_ty.specifier == .@"union") {
+    } else if (init_ty.is(.@"union")) {


Vexu · 2021-09-16T17:30:31Z

src/Parser.zig

@@ -1788,7 +1812,7 @@ fn paramDecls(p: *Parser) Error!?[]Type.Func.Param {
        } else if (param_ty.isArray()) {
            // params declared as arrays are converted to pointers
            param_ty.decayArray();
-        } else if (param_ty.specifier == .void) {
+        } else if (param_ty.is(.void)) {


The use of qual.any() below is invalid.

Vexu · 2021-09-16T17:32:33Z

src/Parser.zig

+    if (target.is(.array)) {
+        assert(item.ty.is(.array));


The use of target.data.array.len below should be replaced with arrayLen and the assert should be left as is since item is supposed to be a string literal.

ehaas · 2021-09-16T21:38:54Z

Good catch, that was pretty careless on my part. I'll add the .get method you suggested and try to write some tests to cover the various places where the wrong union field would be used.

ehaas · 2021-09-18T00:22:03Z

Couple questions:

Should arrayLen be defined for static_array? The original code didn't try to get the length of any static arrays but maybe it should be defined?
I believe some of the qualifier code may be incorrect with initializers + incomplete arrays and also with conditional expressions - pulling the qualifier from the unwrapped type. Do we need a method that merges all the qualifiers from the child types of typeof-types?

Vexu · 2021-09-18T07:10:22Z

Couple questions:

1. Should `arrayLen` be defined for `static_array`? The original code didn't try to get the length of any static arrays but maybe it should be defined?

It doesn't matter for initializers as the init_ty can never be a static array but if you do include them then the function can also be used in checkArrayBounds.

2. I believe some of the qualifier code may be incorrect with initializers + incomplete arrays and also with conditional expressions - pulling the qualifier from the unwrapped type. Do we need a method that merges all the qualifiers from the child types of typeof-types?

Not sure I understand, can you give examples?

Also you seem to have accidentally commited a binary, could you rebase that away?

ehaas · 2021-09-18T17:31:43Z

Not sure I understand, can you give examples?

Specifically thinking of line 2992 in Parser.zig adjusted_elem_ty.qual = a_elem.qual.mergeCV(b_elem.qual); and line 995 in Parser.zig .qual = incomplete_arr_type.qual,

ehaas · 2021-09-18T17:34:11Z

Any idea why the double-free would only happen on windows? I can dig out a windows box for debugging but it would be nice if it happened on mac or linux too :)

Vexu · 2021-09-18T18:03:18Z

Specifically thinking of line 2992 in Parser.zig adjusted_elem_ty.qual = a_elem.qual.mergeCV(b_elem.qual); and line 995 in Parser.zig .qual = incomplete_arr_type.qual,

995 is definitely wrong, for example:

void foo(void) {
    const typeof(int[]) arr = {1, 2, 3};
    // var: '[3]int' should be [3]const int
    //   name: arr
}

I don't see anything wrong with 2992 though.

Any idea why the double-free would only happen on windows?

No idea, I wouldn't be surprised if it was a bug in Zig's std.

ehaas · 2021-09-18T18:25:25Z

I'll get my Windows machine up to date and see if I can at least bisect it to one of my commits. Will also double check the qualifiers.

Add two new type specifiers: `typeof_type` and `typeof_expr`, which are the types returned by `typeof` (depending on whether it's called with a type or an expression) This allows us to track the underlying type or expression that was used.

Allows typeof() types to function correctly with the parser

ehaas · 2021-09-25T05:40:36Z

Ok, I updated things to make the qualifiers work better now, although I'm not super happy with the canonicalize function - the preserve_quals option is only used in the elemType function, to pick up all the qualifiers that were added by various levels of typeof(); and it's only used to get the qualifiers and not the type itself. So it seems like it would possibly make sense to have a separate getQuals function for doing that, but most of the logic would be similar to that in the canonicalize function.

I get the sense that there is something I'm missing that would simplify this; possibly in the data representation.

Vexu · 2021-09-27T15:22:43Z

Thanks! That seems like something that could be improved but it's probably not worth investigating before attributed types are implemented.

Vexu approved these changes Sep 15, 2021

View reviewed changes

ehaas force-pushed the improve-typeof branch 2 times, most recently from 91ce614 to c375f36 Compare September 16, 2021 06:09

ehaas marked this pull request as ready for review September 16, 2021 06:10

Vexu requested changes Sep 16, 2021

View reviewed changes

ehaas force-pushed the improve-typeof branch from c375f36 to cc18d57 Compare September 18, 2021 00:18

ehaas force-pushed the improve-typeof branch from cc18d57 to 7d7a8fe Compare September 18, 2021 17:20

Vexu force-pushed the improve-typeof branch from 7d7a8fe to d2ad397 Compare September 24, 2021 07:17

ehaas added 10 commits September 24, 2021 10:59

Parser: improve typeof support

e3dc054

Add two new type specifiers: `typeof_type` and `typeof_expr`, which are the types returned by `typeof` (depending on whether it's called with a type or an expression) This allows us to track the underlying type or expression that was used.

Type: Add .is method for checking specifier

26f4cf3

Allows typeof() types to function correctly with the parser

Rename Type.VLA to Type.Expr

225c0b4

Parser: do not inherit register with typeof

dce4fc2

Type: Add arrayLen method

d35de5a

add .get() method and deal with union/struct init

d6ecc30

Parser: handle incomplete array init with typeof

9984a19

Type: add anyQual method for checking qualifiers on meta types

4711b0f

Parser: use .get() method when necessary

11fc930

Update arrayLen to work with more types

985545c

ehaas force-pushed the improve-typeof branch from d2ad397 to 39dfcca Compare September 25, 2021 05:25

ehaas added 2 commits September 24, 2021 23:29

Type: ensure qualifiers are correct for typeof-types

18bf7e0

Type: use .is() functions for eitherLongDouble etc

8c746e5

ehaas force-pushed the improve-typeof branch from 39dfcca to 8c746e5 Compare September 25, 2021 05:29

Vexu merged commit 8c5f625 into Vexu:master Sep 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parser: improve typeof support #27

Parser: improve typeof support #27

ehaas commented Sep 13, 2021

ehaas commented Sep 13, 2021

Vexu left a comment

Vexu Sep 15, 2021

ehaas Sep 16, 2021

Vexu Sep 15, 2021

Vexu left a comment

Vexu Sep 16, 2021

Vexu Sep 16, 2021

Vexu Sep 16, 2021

Vexu Sep 16, 2021

Vexu Sep 16, 2021

Vexu Sep 16, 2021

Vexu Sep 16, 2021

Vexu Sep 16, 2021

Vexu Sep 16, 2021

ehaas commented Sep 16, 2021

ehaas commented Sep 18, 2021

Vexu commented Sep 18, 2021

ehaas commented Sep 18, 2021

ehaas commented Sep 18, 2021

Vexu commented Sep 18, 2021

ehaas commented Sep 18, 2021

ehaas commented Sep 25, 2021

Vexu commented Sep 27, 2021

Parser: improve typeof support #27

Parser: improve typeof support #27

Conversation

ehaas commented Sep 13, 2021

ehaas commented Sep 13, 2021

Vexu left a 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

Vexu left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehaas commented Sep 16, 2021

ehaas commented Sep 18, 2021

Vexu commented Sep 18, 2021

ehaas commented Sep 18, 2021

ehaas commented Sep 18, 2021

Vexu commented Sep 18, 2021

ehaas commented Sep 18, 2021

ehaas commented Sep 25, 2021

Vexu commented Sep 27, 2021