Uh oh!
There was an error while loading. Please reload this page.
Allow accessors to override non-class or abstract properties - #41994
Allow accessors to override non-class or abstract properties #41994Nathan Shively-Sanders (sandersn) merged 15 commits into
Conversation
Based on discussion in the design meeting, the change should instead be to exempt all properties that don't come from a class. Plus, we should keep this issue in mind in case other changes to Ecmascript cause TS to add language features dealing with prototypiness of properties/accessors. |
Nathan Shively-Sanders (sandersn)
commented
Feb 9, 2021
This now matches what we decided in the design meeting. I'm going to fold in #42635 next since they do almost the same thing. |
Originally from #42635, but this version is simpler.
Eli Barzilay (@elibarzilay) do you mind taking a look at this while Ron Buckton (@rbuckton) is at TC39? |
Ron Buckton (rbuckton)
commented
May 10, 2022
I'm not 100% sure what "all properties" means here. We specifically added support to describe getters and setters in interfaces and type literals to differentiate between fields and accessors. We couldn't do that for mapped types, though, since mapped types can only describe field-like values (i.e., no getters, setters, or methods). |
Re-reading it, I think I put the specifier in the wrong location on that comment -- I edited the description a couple months after the design meeting, so it's more up-to-date than the comment. Basically, the change should exempt properties that whose declarations all come from outside a class, unless they're synthetic, in which case they are exempt if any declaration comes from outside a class. I don't know what support we had for accessors in interfaces and type literals at the time, but it definitely sounds like they behave the same as classes now. I'll update the code to implement that. |
Seems simple and doesn't break much. I need to double-check the few test failures, however.
Instead of looping over the props list 3 more times.
Well, running the tests again without the accessor change in createUnionOrIntersectionProperty still has them all passing. So I reverted my change to see whether CI agrees with me. |
Ron Buckton (rbuckton)
left a comment
There was a problem hiding this comment.
This looks fine, though I have a few questions.
| // Mixin base class | ||
| interface ApiItemContainerMixin extends ApiItem { | ||
| readonly members: ReadonlyArray<ApiItem>; |
There was a problem hiding this comment.
I'm curious why this is readonly members: ReadonlyArray<ApiItem>; and not get members(): ReadonlyArray<ApiItem>;. Is it purely to support pre-TS 4.0, which wouldn't be able to parse the declaration file?
4.0 has been available for over a year and a half, and 41347 was filed only two months after 4.0 shipped. If we are weakening restrictions to improve backcompat support, I'd want to be sure the backcompat support is warranted. If api-extractor still supports consumers using < TS 4.0, then I could see this being valuable.
There was a problem hiding this comment.
Pete Gonzalez (@octogonz), is pre-TS 4.0 support still critical to api-extractor package consumers?
There was a problem hiding this comment.
It's still an error because of the way union/intersection properties are created. If I bring back the commit with create-unions-of-accessors-as-accessors, your suggestion might work. Let me check.
There was a problem hiding this comment.
Nope, the changed check is still required. (It's not that much looser either; it just allows any declaration of a synthetic symbol instead of only valueDeclaration).
| function getNonInterhitedProperties(type: InterfaceType, baseTypes: BaseType[], properties: Symbol[]) { | ||
| function isPropertyAbstractOrInterface(declaration: Declaration, baseDeclarationFlags: ModifierFlags) { | ||
| return baseDeclarationFlags & ModifierFlags.Abstract && (!isPropertyDeclaration(declaration) || !declaration.initializer) | ||
| || isInterfaceDeclaration(declaration.parent); |
There was a problem hiding this comment.
Should we only care that the declaration is an interface? I find it interesting that the following isn't currently an error:
declareclassX1{gety(): number;}classY1extendsX1{y=1;// error (correct)}interfaceX2{gety(): number;}declarevarX2: {new(): X2;prototype: X2;}classY2extendsX2{y=1;// no error (incorrect?)}Maybe we can be weaker when the property on the interface is just PropertySignature because of back-compat, but we probably should be erroring if the property on the interface is a GetAccessorDeclaration or SetAccessorDeclaration and you're trying to override it with a field.
There was a problem hiding this comment.
That's a good idea, but without the commit with create-unions-of-accessors-as-accessors, it adds a weird wrinkle when synthetic properties are all accessors: specifically, if you modify accessorsOverrideProperty9 so that all bases use accessors (as in your other comment), then the error shows up again: the base symbol is synthetic, but all 3 bases are accessors, so the exemption no longer applies.
Instead, execution falls through to the flags check, but the flags are wrong, so the error message is wrong.
It's probably more correct to change the way synthetic accessors are created, but it's a much further-reaching change, and most of this code is still needed. I'd rather have a more pressing reason than this to change the code that creates synthetic accessors.
An accessor may override a base property if all the property's declarations are (1) from an interface or (2) abstract. If the base property is synthetic — from an intersection, for example — then it is allowed if any of the property's declarations are abstract or from an interface.
fix#40733 and fix#41347