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.

Sign up for GitHub

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

Fix complex property globs #1610

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Fix complex property globs #1610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion source/parser.hera
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1735,6 +1735,10 @@ PropertyAccess
}
}

# Property glob that starts with "." or "?.", not a brace
ExplicitPropertyGlob
&ExplicitAccessStart PropertyGlob -> $2

PropertyGlob
# NOTE: Added shorthand obj.{a,b:c} -> {a: obj.a, c: obj.b}
( PropertyAccessModifier? OptionalDot ):dot InlineComment* BracedObjectLiteral:object ->
Expand Down Expand Up @@ -3740,7 +3744,7 @@ MethodDefinition
}
# NOTE: Not adding extra validation using PropertySetParameterList
# NOTE: If this node layout changes, be sure to update `convertMethodTOFunction`
MethodSignature:signature !(PropertyAccess / UnaryPostfix / NonNullAssertion) BracedBlock?:block ->
MethodSignature:signature !(PropertyAccess / ExplicitPropertyGlob / UnaryPostfix / NonNullAssertion) BracedBlock?:block ->
let children = $0
let generatorPos = 0
let { modifier } = signature
Expand Down
82 changes: 37 additions & 45 deletions source/parser/lib.civet
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import type {
MemberExpression
MethodDefinition
NormalCatchParameter
ObjectExpression
ParenthesizedExpression
Placeholder
StatementNode
Expand Down Expand Up @@ -558,13 +559,14 @@ function processCallMemberExpression(node: CallExpression | MemberExpression): A
if glob?.type is "PropertyGlob"
prefix .= children[...i]
parts := []
let refAssignmentComma
let ref
// add ref to ensure object base evaluated only once
if prefix.length > 1
ref := makeRef()
{ refAssignmentComma } = makeRefAssignment ref, prefix
prefix = [ref]
prefix = prefix.concat(glob.dot)
if prefix.length > 1 and glob.object.properties# > 1
ref = makeRef()
{ refAssignment } := makeRefAssignment ref, prefix
// First use of prefix assigns ref
prefix = [ makeLeftHandSideExpression refAssignment ]
prefix = prefix.concat glob.dot

for part of glob.object.properties
if part.type is "Error"
Expand All @@ -575,7 +577,7 @@ function processCallMemberExpression(node: CallExpression | MemberExpression): A
type: "Error"
message: "Glob pattern cannot have method definition"
continue
if part.value and !["CallExpression", "MemberExpression", "Identifier"].includes(part.value.type)
if part.value and part.value.type is not in ["CallExpression", "MemberExpression", "Identifier"] as (string?)[]
parts.push
type: "Error"
message: `Glob pattern must have call or member expression value, found ${JSON.stringify(part.value)}`
Expand All @@ -591,8 +593,10 @@ function processCallMemberExpression(node: CallExpression | MemberExpression): A
// Not yet needed:
[name, value] = [value, name] if glob.reversed

if !suppressPrefix // Don't prefix @ shorthand
unless suppressPrefix // Don't prefix @ shorthand
value = prefix.concat trimFirstSpace value
// Switch from refAssignment to ref
prefix = [ ref ] ++ glob.dot if ref?
if (wValue) value.unshift(wValue)
if part.type is "SpreadProperty"
parts.push {
Expand All @@ -603,6 +607,7 @@ function processCallMemberExpression(node: CallExpression | MemberExpression): A
names: part.names
children: part.children.slice(0, 2) // whitespace, ...
.concat(value, part.delim)
usesRef: Boolean ref
}
else
parts.push {
Expand All @@ -619,21 +624,16 @@ function processCallMemberExpression(node: CallExpression | MemberExpression): A
value
part.delim // comma delimiter
]
usesRef: Boolean ref
}
object: ASTNodeObject .= {
object: ObjectExpression :=
type: "ObjectExpression"
children: [
glob.object.children.0 // {
...parts
glob.object.children.-1 // whitespace and }
],
]
properties: parts
}
if refAssignmentComma
object = makeNode
type: "ParenthesizedExpression"
children: ["(", ...refAssignmentComma, object, ")"]
expression: object
if (i is children.length - 1) return object
return processCallMemberExpression({ // in case there are more
...node
Expand Down Expand Up @@ -842,44 +842,36 @@ function convertNamedImportsToObject(node, pattern?: boolean)
// {foo} is equivalent to foo={foo}, and
// {foo, bar: baz} is equivalent to foo={foo} and bar={baz}.
// {...foo} is a special case.
function convertObjectToJSXAttributes(obj) {
const { properties } = obj
const parts = [] // JSX attributes
const rest = [] // parts that need to be in {...rest} form
for (let i = 0; i < properties.length; i++) {
function convertObjectToJSXAttributes(obj: ObjectExpression)
parts := [] // JSX attributes
rest := [] // parts that need to be in {...rest} form
for part, i of obj.properties
if part.usesRef
rest.push part
continue
if (i > 0) parts.push(' ')
const part = properties[i]
switch (part.type) {
case 'Identifier':
switch part.type
when "Identifier"
parts.push([part.name, '={', part.name, '}'])
break
case 'Property':
if (part.name.type is 'ComputedPropertyName') {
when "Property"
if part.name.type is "ComputedPropertyName"
rest.push(part)
} else {
else
parts.push([part.name, '={', trimFirstSpace(part.value), '}'])
}
break
case 'SpreadProperty':
when "SpreadProperty"
parts.push(['{', part.dots, part.value, '}'])
break
case 'MethodDefinition':
when "MethodDefinition"
const func = convertMethodToFunction(part)
if (func) {
if func
parts.push([part.name, '={', convertMethodToFunction(part), '}'])
} else {
else
rest.push(part)
}
break
default:
throw new Error(`invalid object literal type in JSX attribute: ${part.type}`)
}
}
if (rest.length) {
parts.push(['{...{', ...rest, '}}'])
}
else
throw new Error `invalid object literal type in JSX attribute: ${part.type}`
if rest#
parts.push " " if parts# and parts.-1 is not " "
parts.push(["{...{", ...rest, "}}"])
return parts
}

/**
* Returns a new MethodDefinition node.
Expand Down
11 changes: 10 additions & 1 deletion source/parser/types.civet
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export type OtherNode =
| Placeholder
| PropertyAccess
| PropertyBind
| PropertyGlob
| RangeExpression
| ReturnTypeAnnotation
| ReturnValue
Expand Down Expand Up @@ -497,6 +498,13 @@ export type PropertyBind
name: string
args: ASTNode[]

export type PropertyGlob
type: "PropertyGlob"
children: Children
parent?: Parent
dot: ASTNode
object: ObjectExpression

export type Call
type: "Call"
children: Children
Expand Down Expand Up @@ -848,7 +856,7 @@ export type ObjectExpression
type: "ObjectExpression"
children: Children
names: string[]
properties: Property[]
properties: (Property | SpreadProperty | MethodDefinition | ASTError)[]
parent?: Parent

export type Property
Expand All @@ -858,6 +866,7 @@ export type Property
name: string
names: string[]
value: ASTNode
usesRef?: boolean

export type ArrayExpression
type: "ArrayExpression"
Expand Down
12 changes: 10 additions & 2 deletions test/import.civet
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,12 +333,20 @@ describe "import", ->
}
"""

testCase """
single dynamic import declaration expression
---
fs := import { readFileSync } from fs
---
const fs = {readFileSync:await import("fs").readFileSync}
"""

testCase """
dynamic import declaration expression
---
fs := import { readFileSync, writeFile as wf, writeFileSync: wfs } from fs
---
let ref;const fs = (ref = await import("fs"),{readFileSync:ref.readFileSync,wf:ref.writeFile,wfs:ref.writeFileSync})
let ref;const fs = {readFileSync:(ref = await import("fs")).readFileSync,wf:ref.writeFile,wfs:ref.writeFileSync}
"""

throws """
Expand All @@ -354,7 +362,7 @@ describe "import", ->
---
data := import { version } from package.json with type: 'json'
---
let ref;const data = (ref = await import("package.json", {with:{type: 'json'}}),{version:ref.version})
const data = {version:await import("package.json", {with:{type: 'json'}}).version}
"""

// #1307
Expand Down
16 changes: 16 additions & 0 deletions test/jsx/attr.civet
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,22 @@ describe "braced JSX attributes", ->
<Component x={{b:a.b, c:a.c}} />
"""

testCase """
glob with complex left-hand side
---
<Component {f()?.{nx,ny}}>
---
let ref;<Component {...{nx:(ref = f())?.nx,ny:ref?.ny}} />
"""

testCase """
glob with complex left-hand side and more
---
<Component {a, f()?.{nx,ny}, b}>
---
let ref;<Component a={a} b={b} {...{ nx:(ref = f())?.nx,ny:ref?.ny,}} />
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to preserve the order here if possible. I believe it can have an impact if the same key also appears in the spread.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. This is an issue "inherited" from my old JSX attribute code, which e.g. compiles <Foo {a, [b]: x, c}> to <Foo a={a} c={c} {...{ [b]: x }} />. But it shouldn't be too hard to fix, by accumulating maximal chunks that need {...{ treatment. I'll leave it for a future PR for easier review.

"""

testCase """
bind shorthand
---
Expand Down
22 changes: 19 additions & 3 deletions test/object.civet
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,7 +1285,15 @@ describe "object", ->
---
x.y()?.z.{a,b}
---
let ref;(ref = x.y()?.z,{a:ref.a,b:ref.b})
let ref;({a:(ref = x.y()?.z).a,b:ref.b})
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be out of scope for this particular fix but if x.y() ends up undefined then .a and .b on the ref will throw instead of being short circuited.

Also related to that we should decide what to return in the case of the ref not resolving. I think undefined is correct but in the previous example with the ?. directly before the glob we're returing {nx: undefined, ny: undefined}

I think the check will end up being something like ((ref = x.y()) != null ? ((ref=ref.z), { a:ref.a, b:ref.b }) : ref

Sorry it's dense but note the use of the comma operator inside the first ternary branch to manually update the ref. We may need to do it for each time ?. appears before the glob.

Essentially manually short circuiting every ?. along the chain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, good point: this is probably unexpected behavior that x?.y.{a,b} crashes when x is undefined. The behavior is not changed by this PR, but this PR's compilation does make it potentially more difficult

One challenge with your compilation is that globs can appear inside object expressions, e.g. {c, x?.y.{a,b}}. That's why I switched to setting the ref here to get set in the first element instead of outside in a parenthesized expression; the latter wasn't properly mixable. But of course it's possible to combine the two strategies as you say.

A simpler workaround, but with different behavior, would be to compile x()?.y.z.{a,b} into {a: (ref = x()?.y.z)?.a, b: ref?.b}. In other words, add a ? after ref if there was any ? earlier in the chain. But this would return {a: undefined, b: undefined} instead of undefined. I'm not totally sure which behavior is better... I could see preferring {a: undefined, b: undefined} if e.g. doing the following:

{a, b} := object.{a,b}

Although, in that case, we could just write {a, b} := object. Maybe a better example is returning a projected object? But again it's unclear which is better/more intuitive: returning undefined or an object containing undefineds... What do you think?

Copy link
Contributor

@STRd6 STRd6 Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think ultimately we'll need to short circuit at exactly each ?. The ones outside the glob prevent adding any keys, the ones inside the glob add the key but its value may be undefined.

Checking later in aggregate wouldn't quite be semantically correct. In this case x?.y when x is defined but y is undefined should throw.

Spacing for clarity:

{c, x?.y.{a,b}}
---
let ref;
({ 
  c, 
  ...( (ref = x) != null && (    // short circuit at `x?.`
    ref = ref.y,    // update ref
    {a: ref.a, b: ref.b}    // object for spread
  ) )
})

For the case of projecting I think we'd want to explicitly show that we'd prefer to ignore the missing object rather than to absorb the error. TS will also warn us if the value may potentially be undefined.

{a, b} := {...object?.{a,b}}

Though a case could be made that the ?. immediately preceding the glob {...} may prefer constructing the projection anyway.

Copy link
Collaborator Author

@edemaine edemaine Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting. The {...glob} splat perspective is clarifying: if the glob is undefined (because of an earlier failure), it doesn't add anything to the object. I guess this is somewhat in line with our recent object comprehensions too (which are kinda like implicit ... splats).

I'm still not totally sure which interpretation is correct, but I'm coming around to and you're probably right that undefined is more natural behavior than {a: undefined, b: undefined}.

Note that if we want the latter, we could avoid ... I think. (I do agree that we need to check each possible failure separately though; my ?. attempt is indeed broken.)

{c, x()?.y.{a,b}}
---
let xref, yref;
({
  c,
  a: (xref = x()) == null ? undefined :
    (yref = xref.y).a,
  b: xref == null ? undefined : yref.b
})

"""

testCase """
no ref if single right-hand side
---
a.b.{x}
---
({x:a.b.x})
"""

testCase """
Expand Down Expand Up @@ -1356,15 +1364,15 @@ describe "object", ->
---
f a.b.{x,y}
---
let ref;f((ref = a.b,{x:ref.x,y:ref.y}))
let ref;f({x:(ref = a.b).x,y:ref.y})
"""

testCase """
ref in multi-argument function call
---
f first, a.b.{x,y}, last
---
let ref;f(first, (ref = a.b,{x:ref.x,y:ref.y}), last)
let ref;f(first, {x:(ref = a.b).x,y:ref.y}, last)
"""

throws """
Expand Down Expand Up @@ -1421,6 +1429,14 @@ describe "object", ->
({a:x.a,b:x.b, c:y.c,d:y.d})
"""

testCase """
two inside braced object literals with complex base
---
{x().{a,b}, y()?.{c,d}}
---
let ref;let ref1;({a:(ref = x()).a,b:ref.b, c:(ref1 = y())?.c,d:ref1?.d})
"""

testCase """
with reserved word keys
---
Expand Down
Loading