Avoid binding readonly properties as special properties on a function symbol #49146
Conversation
src/compiler/binder.ts
Outdated
| if (!isInJSFile(node) && !isFunctionSym) { | ||
| return; | ||
| } | ||
| if (isFunctionSym && getDeclarationName(cast(node.left, isBindableStaticNameExpression)) === "length") { |
There was a problem hiding this comment.
I'm not super happy about this fix but binder doesn't have access any access to the type information so I can't just check what is declared on the interface Function as readonly.
And I think that it's better to avoid binding this property altogether than trying to account for this case in the checker or to write some fixup logic in between the binder and the checker.
I've originally described my conclusion after the debugging session here: #49113 (comment)
|
|
||
| f0.length = 1; | ||
| >f0.length = 1 : 1 | ||
| >f0.length : any |
There was a problem hiding this comment.
this feels wrong - perhaps the .types printer tries to only lookup the "target" symbol of this assignment and doesn't try to look into Function here etc. Should I try to fix this as part of this PR?
There was a problem hiding this comment.
This may be fine. What the type writer is saying is that the type of the expression f0.length = 1 is 1, which is correct:
// note: NOT in strict mode. In strict mode, attempting to assign to length will throw
function f(a, b) { }
console.log(f.length); // 2
const i = f.length = 1;
console.log(i); // 1
console.log(f.length); // 2There was a problem hiding this comment.
I was specifically referring to the 9th line that has f0.length : any and it seems that this any is a little bit off here as I would expect this to be of type number 🤔
There was a problem hiding this comment.
Ah, but actually this is already the case for new Function case so perhaps this is how it's printed for the readonly LHS. It's still a little bit confusing to me - but at least my PR doesn't really change anything in this regard 🤷♂️
Andarist
force-pushed
the
fix/fn-readonly-length
branch
from
b390e46 to
6046ffb
Compare
src/compiler/binder.ts
Outdated
| if (!isInJSFile(node) && !isFunctionSym) { | ||
| return; | ||
| } | ||
| if (isFunctionSym && isBindableStaticNameExpression(node.left) && getDeclarationName(node.left) === "length") { |
There was a problem hiding this comment.
Ideally we would be able to query the type of Function ahead of time, but this is done in the binder. Is there some machinery to do this later in the checker? @rbuckton @ahejlsberg
There was a problem hiding this comment.
We have the ability to introduce new bindings in the checker for properties coming from literal types or unique symbol types, but not for this case.
This seems like an acceptable way to do this, since length is a special case per spec (since it is writable: false, configurable: true). We should probably also do this for name, since it has the same restrictions.
There was a problem hiding this comment.
Yeah, name was what I was thinking of. I just generally don't like these special-cases.
There was a problem hiding this comment.
I've extended the logic to handle name as well.
There was a problem hiding this comment.
The whole concept of "special property assignments" is a special case. In for a penny...
Andarist
changed the title
length property as special property on a function symbol
|
I don't think the right fix is to modify the binder. My rough sketch of a fix here would be to, when synthesizing the property from the SPA, also include normal name resolution that can find apparent properties, and mix the flags of the two symbols correctly. It's possible this isn't worth fixing given the very small number of times it comes up vs how often we'd have to do the sort of work implied by either fix. |
|
What does synthesizing mean in this context? How I would recognize there that a property comes from SPA? How one can combine multiple name resolutions? I've thought that a property lookup is recursive but that it only checks a single "layer" at a time. Is there any such thing that you are describing anywhere in the codebase so I could take a look there to understand it better? Or would this be a new mechanism? |
|
Do we have any consensus on how I should proceed here? What kind of a solution is preferred? |
# Conflicts: # src/compiler/binder.ts
|
I think this is the right fix. It is a special-case, but it's one-based on the spec, similar to the weird way the two-pass binding of functions first to simulate lifting. Ryan's solution doesn't work well because property-assignment declarations happen in the binder, which doesn't have the ability to look up apparent types. Once the property is bound, it's hard to distinguish from any other property; you'd have to special-case function, class and object types in the checker to prioritise apparent-type lookup of properties before own properties. @Andarist this has been sitting a while. Do you want to revive this? It's not high priority, obviously, or we wouldn't have it let it sit for so long. |
Yes, I can revive this soon-ish and ping you for re-review then. |
fixes #49113