-
Notifications
You must be signed in to change notification settings - Fork 760
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
Allow dot property access on union of objects with undeclared property #14060
Conversation
Test this change out locally with the following install scripts (Action run 9066237433) VSCode
Azure CLI
|
// TODO could consider returning the null type here, but need to be careful about downstream impact. | ||
// See https://github.com/Azure/bicep/issues/14059 for example |
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.
@jeskew what are your thoughts on the risk here?
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.
I think we do want to use an explicit null
, since we have linters that will propose a fix from <base>.<property>
to <base>.?<property>
if the property has a type like string | null
. But this should only occur if <base>
is an object. So the type of foo.bar
should be the following (mapping typeof(foo)
=> typeof(foo.bar)
):
{ bar: string } | { bar: int }
=>string | int
{ bar: string } | {}
=>string | null
{ otherProp: string } | { otherProp: int }
=>error
{ bar: string } | string
=>error
Like you say in the comment, though, we can use any
now and refine it later if you want to get this out immediately.
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.
Thanks! I think I'd like to leave it more permissive for now, because making this change breaks the 2 samples given in the issue report.
In particular, this becomes quite annoying to handle:
var items = [
{ obj: 1 }
{ obj: 2 }
{ obj: 3 }
{ obj: 4 }
{ obj: 5, x: 5 }
]
var s = sort(
filter(
map(items, x => x.obj < 2 ? { order: 1, value: x } : x.obj > 4 ? { order: 2, value: x } : {}),
x => !(empty(x))
),
(arg1, arg2) => arg1.order < arg2.order
)
Because the impact on types of !(empty(x))
isn't understood by the filter
function, you end up having to do a lot of work in the sort
function - e.g. I think you'd either need (arg1, arg2) => any(arg1).order < any(arg2).order
or (arg1, arg2) => (arg1.?order ?? 0) < (arg2.?order ?? 0)
22f030b
to
808c67c
Compare
Closes #14059
Microsoft Reviewers: Open in CodeFlow