Display argument placeholders for autoscoped constructors #9737
Conversation
|
Please also test |
Testing by hand produces weird results: 🤔 
And no argument placeholders are visible, unlike a regular constructor call: 
We don’t receive any method call info from the engine there. What do you mean by testing? Adding another e2e test case? It’s not clear to me how we can close #9417, as it requires additional changes to widget selection. |
app/gui2/shared/ast/tree.ts
Outdated
| } | ||
| } | ||
| export interface MutableAutoscopedIdentifier extends AutoscopedIdentifier, MutableAst { | ||
| get identifier(): MutableAst | undefined |
There was a problem hiding this comment.
This type of declaration for MutableAst subtypes is used to convey to the type system a property that we know, but can't generically express at type level: If an Ast field is accessed from a MutableAst, the field itself is also a MutableAst. The declaration refines the type of a getter defined on the non-mutable Ast subtype.
In this case, there is no corresponding implementation in the Ast subtype for this declaration to refine, but also it would not be possible to implement one--The identifier is a bare token, not an expression, so we can't provide MutableAst access to it.
app/gui2/shared/ast/tree.ts
Outdated
| declare readonly module: MutableModule | ||
| declare readonly fields: FixedMap<AstFields & AutoscopedIdentifierFields> | ||
|
|
||
| setOperator(value: Token) { |
There was a problem hiding this comment.
There is only one valid operator for an autoscoped identifier (..), so I don't see a need for this interface.
app/gui2/shared/ast/tree.ts
Outdated
| setOperator(value: Token) { | ||
| this.fields.set('operator', unspaced(value)) | ||
| } | ||
| setIdentifier(value: Token) { |
There was a problem hiding this comment.
It's important that we don't allow any mutations that would break the syntax--we must only accept values that, if printed and re-parsed, would still yield an AutoscopedIdentifier. In this case the only legal operands are capitalized Identifiers; a subtype of Identifier could be defined to enforce this restriction, maybe called TypeOrConstructorIdentifier (I have never been able to come up with a less awkward name for the things that are capitalized in Enso syntax).
app/gui2/shared/ast/tree.ts
Outdated
| return asOwned(new MutableAutoscopedIdentifier(module, fields)) | ||
| } | ||
|
|
||
| static new(module: MutableModule, operator: Token, identifier: Token) { |
There was a problem hiding this comment.
The new methods for Ast types are the safe, easy to use interface for creating instances (as opposed to concrete, which is for exactly-representing any data provided by the parser). As such there are a few things I'd do differently here:
- Since only one operator would be correct, rather than accept an operator
Tokenit can be constructed in the method. - Arbitrary tokens can't safely be accepted for the
identifier, see comment onsetIdentifierbelow. - The
moduleshould be an optional parameter at the end (constructing aMutableModule.Transient()if omitted)--currently types are not consistent about this, but I have been moving toward this more convenient API.
vitvakatu
added
CI: Ready to merge
Pull Request Description
Fixes #9635
Requesting review from @kazcw, because I touched parser code.
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build.