-
Notifications
You must be signed in to change notification settings - Fork 787
Add interfaces as a proof-of-concept experimental feature #10181
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
base: master
Are you sure you want to change the base?
Conversation
Add a new ElementType variant that will allow us to parse interface declarations using the existing element parsing code. The match arms implemented are my best guess based on the context of the enclosing function and how a global is handled.
A document may contain zero or more interfaces. Interfaces do not support inheritance.
Verify that an interface cannot have: - sub elements; - repeated elements; - property animations; - states; - transitions; - an init callback declaration; - an init callback implementation.
Warn if private properties are declared.
It is treated as an alias for `inherits`, for now.
The compiler produces an error when the `implements` keyword is used before a global, component, or builtin type. The parser doesn't know about this kind of relationship - the "implements" and "inherits" keywords are just consumed. For now we just go back and check what the expected relationship type was whilst constructing an Element.
Attempt to verify that the interpreter can see and use properties declared in an interface that a component implements. This is an initial proof-of-concept, we will need to come back and add checks and tests for mixing 'inherits' with 'implements'.
Group interface related tests in a subdirectory of their own.
Allow a new component to specify that it exposes one or more interfaces via a given child element. This commit introduces the syntax, we will add the implementation in a future commit.
For each uses statement, iterate through the interface properties and create two-way bindings to the property on the specified base component. The error cases will be populated in follow-up commit. For now, demonstrate that the initial concept works.
Detect and emit errors where: - the type specified in a uses statement is not an interface; - the type specified in a uses statement is unknown; - the type specified in a uses statement cannot be used in this context.
…does not exist Add a test case to verify the expected error message.
Add a test to verify that interfaces from imported modules can be used.
Add a test case to verify that: - we can import an interface as another name and re-use it; - we can import a component that implements a renamed interface and use it.
For each property in an interface that a child element is expected to implement, verify that the child has a property with the same name, type and visibility. This is easier than attempting to store and match interface names which may change if the user renames an interface when importing.
Emit a compile error if the user attempts to declare a property that would override a property from an interface.
…an inherited property Look up the interface property on the base type before allowing a component to implement the interface.
…xisting binding I couldn't figure out a good way to test this - everything I came up with got caught by the prior check that the property does not already exist.
Guard these keywords behind the experimental features flag. Slint must be compiled with the `SLINT_ENABLE_EXPERIMENTAL_FEATURES` environment variable set to enable these keywords.
| { | ||
| r.property_declarations.insert(prop_name.clone(), prop_decl.clone()); | ||
| } | ||
| } |
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.
This likely needs more checks, e.g. that the properties do not already existed on base components.
| }); | ||
|
|
||
| if base_type == ElementType::Interface && visibility == PropertyVisibility::Private { | ||
| diag.push_warning( |
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.
Should be an error.
| let message = format!( | ||
| "Cannot override binding for property '{}' from interface '{}'", | ||
| prop_name, uses_statement.interface_name | ||
| ); |
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 struggled to come up with a test case for this error - the previous checks caught it first. Suggestions would be appreciated.
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 this would be something like
property <XXX> foo: something;
foo: something_else;Or, in your case:
... use ... (that has a property foo with a binding)
foo: something_else;Unclear if this should be an error actually.
| if p.peek().kind() == SyntaxKind::ColonEqual { | ||
| p.warning("':=' to declare a global is deprecated. Remove the ':='"); | ||
| let description = if is_global { "a global" } else { "an interface" }; | ||
| p.warning(format!("':=' to declare {description} is deprecated. Remove the ':='")); |
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.
Should be an error for interfaces?
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'd say so.
ogoffart
left a comment
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.
Looks very good, thank you.
I think that apart from the failling CI, this could actually be merged.
I just wrote some comments that could make the patch even better.
| interface_name_node: syntax_nodes::QualifiedName, | ||
| child_id: SmolStr, | ||
| child_id_node: syntax_nodes::DeclaredIdentifier, |
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.
Not a big issue, but maybe it's enough to have only one node like:
| interface_name_node: syntax_nodes::QualifiedName, | |
| child_id: SmolStr, | |
| child_id_node: syntax_nodes::DeclaredIdentifier, | |
| node: syntax_nodes::UsesIdentifier, | |
| child_id: SmolStr, |
As it is trivial to access the child node with node.QualifiedName() and node.DeclaredIdentifier()
| /// `uses { Foo from Bar, Baz from Qux }` | ||
| UsesSpecifier -> [ UsesIdenfifierList ], | ||
| UsesIdenfifierList -> [ *UsesIdentifier ], |
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.
Minor again, but I'm thinking we don't need the USeIdentifierList node and we can have
| /// `uses { Foo from Bar, Baz from Qux }` | |
| UsesSpecifier -> [ UsesIdenfifierList ], | |
| UsesIdenfifierList -> [ *UsesIdentifier ], | |
| /// `uses { Foo from Bar, Baz from Qux }` | |
| UsesSpecifier -> [ UsesIdentifier ], |
| if p.peek().kind() == SyntaxKind::ColonEqual { | ||
| p.warning("':=' to declare a global is deprecated. Remove the ':='"); | ||
| let description = if is_global { "a global" } else { "an interface" }; | ||
| p.warning(format!("':=' to declare {description} is deprecated. Remove the ':='")); |
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'd say so.
| fn try_from( | ||
| node: &syntax_nodes::UsesIdentifier, | ||
| ) -> Result<Self, <Self as TryFrom<&syntax_nodes::UsesIdentifier>>::Error> { | ||
| let interface_name_node = node.child_node(SyntaxKind::QualifiedName).ok_or(())?; |
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.
You should be able to do node.QualifiedName()
| &base_node, | ||
| ); | ||
| ElementType::Error | ||
| } else if !diag.enable_experimental { |
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.
(maybe this should be the first condition?)
| let message = format!( | ||
| "Cannot override binding for property '{}' from interface '{}'", | ||
| prop_name, uses_statement.interface_name | ||
| ); |
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 this would be something like
property <XXX> foo: something;
foo: something_else;Or, in your case:
... use ... (that has a property foo with a binding)
foo: something_else;Unclear if this should be an error actually.
| } | ||
| } | ||
|
|
||
| /// Check that the given element implements the given interface. Emits a diagnostic if the interface is not implemented. |
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.
So this is like implicit interface where the components itself didn't explicitly sayd implements <...>
I'm wondering if interface implementation shouldn't be explicit.
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.
Yes. My initial approach was to store the list of interfaces that were implemented as a member of the element (i.e. interfaces should be explicit). That wasn't working because the list was getting lost/not available where I was expecting it, so I switched to this approach for the proof-of-concept. I'd be happy to look at explicit interfaces as a follow-up.
| /// component F uses { I from A } implements J { } | ||
| /// component F uses { I from A } inherits B { } | ||
| /// component F uses { I from A, J from B } { } | ||
| /// component F uses { I from A, J from B } implements J { } | ||
| /// component F uses { I from A, J from B } inherits C { } |
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'm not sure we should support having both Implement ans inherits or having several interface.
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 don't see why you wouldn't want to support implements and inherits for the same component. For example,
interface ButtonInterface {
callback clicked;
}
export component MyButton implements ButtonInterface inherits Rectangle {
}means that the both the rectangle and interface APIs are available to the user of MyButton. Alternatively we have to verbosely duplicate all the API we want to expose from MyButton, which kind of defeats the purpose of the interface.
I also don't see why you wouldn't want to be able to expose more than one interface. In principle std-widgets might want to expose at least two: the particular std-widget interface, and an accessibility interface.
| let mut p = p.start_node(SyntaxKind::UsesIdentifier); | ||
|
|
||
| if !parse_qualified_name(&mut *p) { | ||
| return false; |
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 need to have the node aven in case of error, otherwise we unwrap in some places:
| return false; | |
| drop(p.start_node(SyntaxKind::DeclaredIdentifier)); | |
| return false; |
Same thing for the next if
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.
Ah, that was why I ended up with the TryFrom. Thank you.
| } | ||
| ElementType::Global | ElementType::Error => panic!("This should not happen"), | ||
| ElementType::Interface => { | ||
| // TODO: I don't think we should be here - but it is too early to tell. Don't panic though. |
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.
This pass happens at the very end, so this is indeed unreachable.
Proof of concept for #1870.
Introduce a new keyword
interfacewhich allows a user to declare an interface. This follows the same rules as a global. An interface cannot have:Introduce a new keyword
implementswhich allows a user to declare that a component implements an interface. This initial implementation simply copies the interface property declarations into the component.Introduce a new keyword
useswhich allows a user to declare that a parent component implements an interface through a child component. This initial implementation creates two way property bindings between the parent component and the child for each property in the interface.This PR only deals with simple property declarations. Future PRs will be needed to: