Sema: do not force resolution of struct field inits when calling function pointer field

[mlugg]

b3462b7 caused a regression in a third-party project, since it forced resolution of field initializers for any field call 'foo.bar()', despite this only being necessary when 'bar' is a comptime field.

See #17692 (comment).

[mlugg]

@linusg, this should fix the kiesel issue you reported on #17692 (kiesel builds locally for me after this change).

[linusg]

Thanks for the quick fix! ❤️

[mlugg]

No problem - I'm currently looking into implementing some error notes to make this kind of error easier to understand in future. In this case, one of the field initializers of InternalMethods is eventually resolving an inferred error set somewhere of a function which calls one of the function pointer fields of InternalMethods - pretty hairy, but probably something that could have been worked around with a better error.

EDIT: okay, just for reference, your exact issue was as follows:

• InternalMethods has field defineOwnProperty with init ordinary_internal_methods.defineOwnProperty
• Since defineOwnProperty (in src/builtins/ordinary.zig) has an inferred error set, this error set must be resolved to coerce to the explicit error set in the field type
• In turn, ordinaryDefineOwnProperty's error set must be resolved
• This function calls getOwnProperty on an internalMethods, triggering this bug

This could be worked around - and the code would be generally improved anyway - by giving explicit error sets to the functions in src/builtins/ordinary.zig.

[linusg]

Thanks for the suggestion - not to turn this into an off-topic discussion, but do you have general recommendations for when to use explicit error sets? The docs more or less say when inferred ones stop working so I'm never really sure 😅

[andrewrk]

Generally, I would say that it is better for a public API to have an explicit error set. For private APIs it kinda doesn't matter, whatever is more convenient for the implementation.

[mlugg]

It's worth noting that explicit error sets are generally friendlier to the compiler - they don't enforce as much ordering on semantic analysis, so avoid issues like this, and will be more open to parallelism in analysis when that's implemented. Andrew's advice is sound, but for private APIs, I generally like to give functions explicit sets in any case where I already have a name for the error set (like in this case).

[linusg]

Great, thanks both :)

[mlugg]

b3462b7 caused a regression in a third-party project, since it forced resolution of field initializers for any field call 'foo.bar()', despite this only being necessary when 'bar' is a comptime field.

See #17692 (comment).

[mlugg]

@linusg, this should fix the kiesel issue you reported on #17692 (kiesel builds locally for me after this change).

[linusg]

Thanks for the quick fix! ❤️

[mlugg]

No problem - I'm currently looking into implementing some error notes to make this kind of error easier to understand in future. In this case, one of the field initializers of InternalMethods is eventually resolving an inferred error set somewhere of a function which calls one of the function pointer fields of InternalMethods - pretty hairy, but probably something that could have been worked around with a better error.

EDIT: okay, just for reference, your exact issue was as follows:

• InternalMethods has field defineOwnProperty with init ordinary_internal_methods.defineOwnProperty
• Since defineOwnProperty (in src/builtins/ordinary.zig) has an inferred error set, this error set must be resolved to coerce to the explicit error set in the field type
• In turn, ordinaryDefineOwnProperty's error set must be resolved
• This function calls getOwnProperty on an internalMethods, triggering this bug

This could be worked around - and the code would be generally improved anyway - by giving explicit error sets to the functions in src/builtins/ordinary.zig.

[linusg]

Thanks for the suggestion - not to turn this into an off-topic discussion, but do you have general recommendations for when to use explicit error sets? The docs more or less say when inferred ones stop working so I'm never really sure 😅

[andrewrk]

Generally, I would say that it is better for a public API to have an explicit error set. For private APIs it kinda doesn't matter, whatever is more convenient for the implementation.

[mlugg]

It's worth noting that explicit error sets are generally friendlier to the compiler - they don't enforce as much ordering on semantic analysis, so avoid issues like this, and will be more open to parallelism in analysis when that's implemented. Andrew's advice is sound, but for private APIs, I generally like to give functions explicit sets in any case where I already have a name for the error set (like in this case).

[linusg]

Great, thanks both :)

[mlugg]

b3462b7 caused a regression in a third-party project, since it forced resolution of field initializers for any field call 'foo.bar()', despite this only being necessary when 'bar' is a comptime field.

See #17692 (comment).

[mlugg]

@linusg, this should fix the kiesel issue you reported on #17692 (kiesel builds locally for me after this change).

[linusg]

Thanks for the quick fix! ❤️

[mlugg]

No problem - I'm currently looking into implementing some error notes to make this kind of error easier to understand in future. In this case, one of the field initializers of InternalMethods is eventually resolving an inferred error set somewhere of a function which calls one of the function pointer fields of InternalMethods - pretty hairy, but probably something that could have been worked around with a better error.

EDIT: okay, just for reference, your exact issue was as follows:

• InternalMethods has field defineOwnProperty with init ordinary_internal_methods.defineOwnProperty
• Since defineOwnProperty (in src/builtins/ordinary.zig) has an inferred error set, this error set must be resolved to coerce to the explicit error set in the field type
• In turn, ordinaryDefineOwnProperty's error set must be resolved
• This function calls getOwnProperty on an internalMethods, triggering this bug

This could be worked around - and the code would be generally improved anyway - by giving explicit error sets to the functions in src/builtins/ordinary.zig.

[linusg]

Thanks for the suggestion - not to turn this into an off-topic discussion, but do you have general recommendations for when to use explicit error sets? The docs more or less say when inferred ones stop working so I'm never really sure 😅

[andrewrk]

Generally, I would say that it is better for a public API to have an explicit error set. For private APIs it kinda doesn't matter, whatever is more convenient for the implementation.

[mlugg]

It's worth noting that explicit error sets are generally friendlier to the compiler - they don't enforce as much ordering on semantic analysis, so avoid issues like this, and will be more open to parallelism in analysis when that's implemented. Andrew's advice is sound, but for private APIs, I generally like to give functions explicit sets in any case where I already have a name for the error set (like in this case).

[linusg]

Great, thanks both :)

[mlugg]

b3462b7 caused a regression in a third-party project, since it forced resolution of field initializers for any field call 'foo.bar()', despite this only being necessary when 'bar' is a comptime field.

See #17692 (comment).

[mlugg]

@linusg, this should fix the kiesel issue you reported on #17692 (kiesel builds locally for me after this change).

[linusg]

Thanks for the quick fix! ❤️

[mlugg]

No problem - I'm currently looking into implementing some error notes to make this kind of error easier to understand in future. In this case, one of the field initializers of InternalMethods is eventually resolving an inferred error set somewhere of a function which calls one of the function pointer fields of InternalMethods - pretty hairy, but probably something that could have been worked around with a better error.

EDIT: okay, just for reference, your exact issue was as follows:

• InternalMethods has field defineOwnProperty with init ordinary_internal_methods.defineOwnProperty
• Since defineOwnProperty (in src/builtins/ordinary.zig) has an inferred error set, this error set must be resolved to coerce to the explicit error set in the field type
• In turn, ordinaryDefineOwnProperty's error set must be resolved
• This function calls getOwnProperty on an internalMethods, triggering this bug

This could be worked around - and the code would be generally improved anyway - by giving explicit error sets to the functions in src/builtins/ordinary.zig.

[linusg]

Thanks for the suggestion - not to turn this into an off-topic discussion, but do you have general recommendations for when to use explicit error sets? The docs more or less say when inferred ones stop working so I'm never really sure 😅

[andrewrk]

Generally, I would say that it is better for a public API to have an explicit error set. For private APIs it kinda doesn't matter, whatever is more convenient for the implementation.

[mlugg]

It's worth noting that explicit error sets are generally friendlier to the compiler - they don't enforce as much ordering on semantic analysis, so avoid issues like this, and will be more open to parallelism in analysis when that's implemented. Andrew's advice is sound, but for private APIs, I generally like to give functions explicit sets in any case where I already have a name for the error set (like in this case).

[linusg]

Great, thanks both :)

[mlugg]

b3462b7 caused a regression in a third-party project, since it forced resolution of field initializers for any field call 'foo.bar()', despite this only being necessary when 'bar' is a comptime field.

See #17692 (comment).

[mlugg]

@linusg, this should fix the kiesel issue you reported on #17692 (kiesel builds locally for me after this change).

[linusg]

Thanks for the quick fix! ❤️

[mlugg]

No problem - I'm currently looking into implementing some error notes to make this kind of error easier to understand in future. In this case, one of the field initializers of InternalMethods is eventually resolving an inferred error set somewhere of a function which calls one of the function pointer fields of InternalMethods - pretty hairy, but probably something that could have been worked around with a better error.

EDIT: okay, just for reference, your exact issue was as follows:

• InternalMethods has field defineOwnProperty with init ordinary_internal_methods.defineOwnProperty
• Since defineOwnProperty (in src/builtins/ordinary.zig) has an inferred error set, this error set must be resolved to coerce to the explicit error set in the field type
• In turn, ordinaryDefineOwnProperty's error set must be resolved
• This function calls getOwnProperty on an internalMethods, triggering this bug

This could be worked around - and the code would be generally improved anyway - by giving explicit error sets to the functions in src/builtins/ordinary.zig.

[linusg]

Thanks for the suggestion - not to turn this into an off-topic discussion, but do you have general recommendations for when to use explicit error sets? The docs more or less say when inferred ones stop working so I'm never really sure 😅

[andrewrk]

Generally, I would say that it is better for a public API to have an explicit error set. For private APIs it kinda doesn't matter, whatever is more convenient for the implementation.

[mlugg]

It's worth noting that explicit error sets are generally friendlier to the compiler - they don't enforce as much ordering on semantic analysis, so avoid issues like this, and will be more open to parallelism in analysis when that's implemented. Andrew's advice is sound, but for private APIs, I generally like to give functions explicit sets in any case where I already have a name for the error set (like in this case).

[linusg]

Great, thanks both :)

[mlugg]

b3462b7 caused a regression in a third-party project, since it forced resolution of field initializers for any field call 'foo.bar()', despite this only being necessary when 'bar' is a comptime field.

See #17692 (comment).

[mlugg]

@linusg, this should fix the kiesel issue you reported on #17692 (kiesel builds locally for me after this change).

[linusg]

Thanks for the quick fix! ❤️

[mlugg]

No problem - I'm currently looking into implementing some error notes to make this kind of error easier to understand in future. In this case, one of the field initializers of InternalMethods is eventually resolving an inferred error set somewhere of a function which calls one of the function pointer fields of InternalMethods - pretty hairy, but probably something that could have been worked around with a better error.

EDIT: okay, just for reference, your exact issue was as follows:

• InternalMethods has field defineOwnProperty with init ordinary_internal_methods.defineOwnProperty
• Since defineOwnProperty (in src/builtins/ordinary.zig) has an inferred error set, this error set must be resolved to coerce to the explicit error set in the field type
• In turn, ordinaryDefineOwnProperty's error set must be resolved
• This function calls getOwnProperty on an internalMethods, triggering this bug

This could be worked around - and the code would be generally improved anyway - by giving explicit error sets to the functions in src/builtins/ordinary.zig.

[linusg]

Thanks for the suggestion - not to turn this into an off-topic discussion, but do you have general recommendations for when to use explicit error sets? The docs more or less say when inferred ones stop working so I'm never really sure 😅

[andrewrk]

Generally, I would say that it is better for a public API to have an explicit error set. For private APIs it kinda doesn't matter, whatever is more convenient for the implementation.

[mlugg]

It's worth noting that explicit error sets are generally friendlier to the compiler - they don't enforce as much ordering on semantic analysis, so avoid issues like this, and will be more open to parallelism in analysis when that's implemented. Andrew's advice is sound, but for private APIs, I generally like to give functions explicit sets in any case where I already have a name for the error set (like in this case).

[linusg]

Great, thanks both :)

[mlugg]

b3462b7 caused a regression in a third-party project, since it forced resolution of field initializers for any field call 'foo.bar()', despite this only being necessary when 'bar' is a comptime field.

See #17692 (comment).

[mlugg]

@linusg, this should fix the kiesel issue you reported on #17692 (kiesel builds locally for me after this change).

[linusg]

Thanks for the quick fix! ❤️

[mlugg]

No problem - I'm currently looking into implementing some error notes to make this kind of error easier to understand in future. In this case, one of the field initializers of InternalMethods is eventually resolving an inferred error set somewhere of a function which calls one of the function pointer fields of InternalMethods - pretty hairy, but probably something that could have been worked around with a better error.

EDIT: okay, just for reference, your exact issue was as follows:

• InternalMethods has field defineOwnProperty with init ordinary_internal_methods.defineOwnProperty
• Since defineOwnProperty (in src/builtins/ordinary.zig) has an inferred error set, this error set must be resolved to coerce to the explicit error set in the field type
• In turn, ordinaryDefineOwnProperty's error set must be resolved
• This function calls getOwnProperty on an internalMethods, triggering this bug

This could be worked around - and the code would be generally improved anyway - by giving explicit error sets to the functions in src/builtins/ordinary.zig.

[linusg]

Thanks for the suggestion - not to turn this into an off-topic discussion, but do you have general recommendations for when to use explicit error sets? The docs more or less say when inferred ones stop working so I'm never really sure 😅

[andrewrk]

Generally, I would say that it is better for a public API to have an explicit error set. For private APIs it kinda doesn't matter, whatever is more convenient for the implementation.

[mlugg]

It's worth noting that explicit error sets are generally friendlier to the compiler - they don't enforce as much ordering on semantic analysis, so avoid issues like this, and will be more open to parallelism in analysis when that's implemented. Andrew's advice is sound, but for private APIs, I generally like to give functions explicit sets in any case where I already have a name for the error set (like in this case).

[linusg]

Great, thanks both :)

[mlugg]

b3462b7 caused a regression in a third-party project, since it forced resolution of field initializers for any field call 'foo.bar()', despite this only being necessary when 'bar' is a comptime field.

See #17692 (comment).

[mlugg]

@linusg, this should fix the kiesel issue you reported on #17692 (kiesel builds locally for me after this change).

[linusg]

Thanks for the quick fix! ❤️

[mlugg]

No problem - I'm currently looking into implementing some error notes to make this kind of error easier to understand in future. In this case, one of the field initializers of InternalMethods is eventually resolving an inferred error set somewhere of a function which calls one of the function pointer fields of InternalMethods - pretty hairy, but probably something that could have been worked around with a better error.

EDIT: okay, just for reference, your exact issue was as follows:

• InternalMethods has field defineOwnProperty with init ordinary_internal_methods.defineOwnProperty
• Since defineOwnProperty (in src/builtins/ordinary.zig) has an inferred error set, this error set must be resolved to coerce to the explicit error set in the field type
• In turn, ordinaryDefineOwnProperty's error set must be resolved
• This function calls getOwnProperty on an internalMethods, triggering this bug

This could be worked around - and the code would be generally improved anyway - by giving explicit error sets to the functions in src/builtins/ordinary.zig.

[linusg]

Thanks for the suggestion - not to turn this into an off-topic discussion, but do you have general recommendations for when to use explicit error sets? The docs more or less say when inferred ones stop working so I'm never really sure 😅

[andrewrk]

Generally, I would say that it is better for a public API to have an explicit error set. For private APIs it kinda doesn't matter, whatever is more convenient for the implementation.

[mlugg]

It's worth noting that explicit error sets are generally friendlier to the compiler - they don't enforce as much ordering on semantic analysis, so avoid issues like this, and will be more open to parallelism in analysis when that's implemented. Andrew's advice is sound, but for private APIs, I generally like to give functions explicit sets in any case where I already have a name for the error set (like in this case).

[linusg]

Great, thanks both :)