Repository navigation
enhance type checker's soundness - #7528
Conversation
| case *ast.FuncNameExpr: | ||
| if f.Name == "ok" { | ||
| if len(call.Args) != 1 { | ||
| panic(call) |
There was a problem hiding this comment.
I think we want to just report an error here as this currently panics: values ok(1,1)
| if recType != nil { | ||
| fields := slices.Clone(recType.Fields) | ||
| if index, ok := recType.IndexOfField(path[0]); ok { | ||
| fields[index].Type = c.putInto(nil, path[1:], leaf) |
There was a problem hiding this comment.
Passing nil into putInto causes any nested record values to be deleted:
$ super -c 'values {a:{c:1}} | put a.b:=2 | values a.c'
no such field "c" at line 1, column 42:
values {a:{c:1}} | put a.b:=2 | values a.c
~
| args, argTypes := t.exprs(call.Args, inType) | ||
| switch f := call.Func.(type) { | ||
| case *ast.FuncNameExpr: | ||
| if f.Name == "ok" { |
There was a problem hiding this comment.
Putting this here also makes it so a udf cannot override a builtin which I suppose is fine but inconsistent with how the rest of the builtin functions work.
There was a problem hiding this comment.
Yeah I don't think you should be able to override builtin ok.
Finish the last sentence in this paragraph of your commit message? |
This commit further enhances super's type soundness by adding logic for checking option values and missing values. Now, many option values usually require ?? or ?. to handle nones (though not everywhere, e.g., record expression and array expression retain the option value). We fixed dot expr to preserve the option type of none values so that they can be properly put into fields with record expressions. For agg function arguments, static type errors are now caught (e.g., missing field) but option values are allowed. It's up to the agg function to decide what to do with option values and option-value nones. For grouping expressions, option values are also allowed and nicely form group-by keys (including option nones) but again, static type errors are caught (including missing field). This means the "ok()??"-pattern should be used in grouping expressions when record fields are expected to be sometimes missing. Function call args are now statically type-checked. We still need to implement a mechnism to infer a return type for functions based on their arg type. In a subsequent PR, we will further improve type-checking error messages and better unify the message text between static and dynamic errors.
| if slices.ContainsFunc(fields, func(f super.Field) bool { | ||
| return f.Type == c.unknown | ||
| }) { | ||
| return c.unknown |
There was a problem hiding this comment.
This means that having a single field with an unknown type (which for instance happens when you use a aggregation) causes entire record to become unknown rather than just constrain the unknown to the unknown field. For example this query is not caught by the semantic checker and the error will be caught in the runtime:
$ super -c 'values {k:"foo"} | aggregate c:=count() by k | values doesnotexist'
error("no such field doesnotexist")
There was a problem hiding this comment.
Oops, I was going after simplicity and now I remember I did things this way for this very reason!
| if !isUnknown(lhs) { | ||
| if hasMissing(lhs) { | ||
| c.error(lloc, errMissing) | ||
| return c.unknown | ||
| } | ||
| if hasOption(lhs) || hasNone(lhs) { | ||
| c.error(lloc, fmt.Errorf("%s: %w", sup.FormatType(lhs), errNone)) | ||
| return c.unknown | ||
| } |
There was a problem hiding this comment.
Strictly speaking, is the !isUnknown guard here necessary?
There was a problem hiding this comment.
(I'm not lobbying for removing it. I just want to make sure I understand.)
4102372 to
3f35347
Compare
|
I'm gonna change |
This commit further enhances super's type soundness by adding logic for checking option values and missing values. Now, many option values usually require ?? or ?. to handle nones (though not everywhere, e.g., record expression and array expression retain the option value). There is still further
We fixed dot expr to preserve the option type of none values so that they can be properly put into fields with record expressions.
For agg function arguments, static type errors are now caught (e.g., missing field) but option values are allowed. It's up to the agg function to decide what to do with option values and option-value nones. For grouping expressions, option values are also allowed and nicely form group-by keys (including option nones) but again, static type errors are caught (including missing field). This means the "ok()??"-pattern should be used in grouping expressions when record fields are expected to be sometimes missing.
Function call args are now statically type-checked. We still need to implement a mechnism to infer a return type for functions based on their arg type.
In a subsequent PR, we will further improve type-checking error messages and better unify the message text between static and dynamic errors.