Hard error when not calling a callback

While before it was silently ignored or caused error in the generated code. Fixes slint-ui#542
task-jp · Oct 5, 2021 · 80d7b2e · 80d7b2e
1 parent 05b7d3e
commit 80d7b2e
Showing 6 changed files with 77 additions and 17 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,8 @@ All notable changes to this project will be documented in this file.
 
  - Due to changes in the build system, the C++ build now requires CMake >= 3.19.
  - Fluent style: The Slider and ScrollBar now updates as the mouse moves.
+ - Parentheses around callable expression is now deprecated.
+ - Naming a callback without calling it is now a hard error instead of producing error in the generated code.
 
 ### Added
 

diff --git a/sixtyfps_compiler/passes/check_expressions.rs b/sixtyfps_compiler/passes/check_expressions.rs
@@ -25,8 +25,9 @@ pub fn check_expressions(doc: &crate::object_tree::Document, diag: &mut BuildDia
 
 fn check_expression(component: &Rc<Component>, e: &Expression, diag: &mut BuildDiagnostics) {
     match e {
-        Expression::MemberFunction { base_node, .. } => {
-            diag.push_error("Member function must be called".into(), base_node);
+        Expression::MemberFunction { .. } => {
+            // Must already have been be reported.
+            debug_assert!(diag.has_error());
         }
         Expression::BuiltinMacroReference(_, node) => {
             diag.push_error("Builtin function must be called".into(), node);

diff --git a/sixtyfps_compiler/passes/resolving.rs b/sixtyfps_compiler/passes/resolving.rs
@@ -243,7 +243,22 @@ impl Expression {
             .map(|n| Self::from_expression_node(n, ctx))
             .or_else(|| node.AtImageUrl().map(|n| Self::from_at_image_url_node(n, ctx)))
             .or_else(|| node.AtLinearGradient().map(|n| Self::from_at_linear_gradient(n, ctx)))
-            .or_else(|| node.QualifiedName().map(|s| Self::from_qualified_name_node(s.into(), ctx)))
+            .or_else(|| {
+                node.QualifiedName().map(|n| {
+                    let exp = Self::from_qualified_name_node(n.clone(), ctx);
+                    if matches!(exp.ty(), Type::Function { .. } | Type::Callback { .. }) {
+                        ctx.diag.push_error(
+                            format!(
+                                "'{}' must be called. Did you forgot the '()'?",
+                                QualifiedTypeName::from_node(n.clone())
+                            )
+                            .into(),
+                            &n,
+                        )
+                    }
+                    exp
+                })
+            })
             .or_else(|| {
                 node.child_text(SyntaxKind::StringLiteral).map(|s| {
                     crate::literals::unescape_string(&s).map(Self::StringLiteral).unwrap_or_else(
@@ -456,9 +471,7 @@ impl Expression {
     }
 
     /// Perform the lookup
-    fn from_qualified_name_node(node: SyntaxNode, ctx: &mut LookupCtx) -> Self {
-        debug_assert_eq!(node.kind(), SyntaxKind::QualifiedName);
-
+    fn from_qualified_name_node(node: syntax_nodes::QualifiedName, ctx: &mut LookupCtx) -> Self {
         let mut it = node
             .children_with_tokens()
             .filter(|n| n.kind() == SyntaxKind::Identifier)
@@ -552,14 +565,30 @@ impl Expression {
         node: syntax_nodes::FunctionCallExpression,
         ctx: &mut LookupCtx,
     ) -> Expression {
-        let mut sub_expr = node.Expression().map(|n| {
-            (Self::from_expression_node(n.clone(), ctx), Some(NodeOrToken::from((*n).clone())))
-        });
-
         let mut arguments = Vec::new();
 
-        let (function, _) =
-            sub_expr.next().unwrap_or_else(|| (Expression::Invalid, Some((*node).clone().into())));
+        let mut sub_expr = node.Expression();
+
+        let function = sub_expr
+            .next()
+            .and_then(|n| {
+                n.QualifiedName().or_else(|| {
+                    n.Expression().and_then(|mut e| {
+                        while let Some(e2) = e.Expression() {
+                            e = e2;
+                        }
+                        e.QualifiedName().map(|q| {
+                            ctx.diag.push_warning("Parentheses around callable are deprecated. Remove the parentheses".into(), &n);
+                            q
+                        })
+                    })
+                })
+            })
+            .map_or(Expression::Invalid, |n| Self::from_qualified_name_node(n.clone(), ctx));
+
+        let sub_expr = sub_expr.map(|n| {
+            (Self::from_expression_node(n.clone(), ctx), Some(NodeOrToken::from((*n).clone())))
+        });
 
         let function = match function {
             Expression::BuiltinMacroReference(mac, n) => {
@@ -917,7 +946,7 @@ impl Expression {
 fn continue_lookup_within_element(
     elem: &ElementRc,
     it: &mut impl Iterator<Item = crate::parser::SyntaxToken>,
-    node: SyntaxNode,
+    node: syntax_nodes::QualifiedName,
     ctx: &mut LookupCtx,
 ) -> Expression {
     let second = if let Some(second) = it.next() {
@@ -947,7 +976,7 @@ fn continue_lookup_within_element(
         let member = elem.borrow().base_type.lookup_member_function(&resolved_name);
         Expression::MemberFunction {
             base: Box::new(Expression::ElementReference(Rc::downgrade(elem))),
-            base_node: Some(node.into()),
+            base_node: Some(NodeOrToken::Node(node.into())),
             member: Box::new(member),
         }
     } else {
@@ -1025,7 +1054,10 @@ pub fn resolve_two_way_binding(
     node: syntax_nodes::TwoWayBinding,
     ctx: &mut LookupCtx,
 ) -> Option<NamedReference> {
-    let e = Expression::from_expression_node(node.Expression(), ctx);
+    let e = node
+        .Expression()
+        .QualifiedName()
+        .map_or(Expression::Invalid, |n| Expression::from_qualified_name_node(n, ctx));
     let ty = e.ty();
     match e {
         Expression::PropertyReference(n) => {

diff --git a/sixtyfps_compiler/tests/syntax/focus/focus_not_called.60 b/sixtyfps_compiler/tests/syntax/focus/focus_not_called.60
@@ -14,11 +14,12 @@ X := Rectangle {
     TouchArea {
         clicked => {
             (edit.focus)();
+//          ^warning{Parentheses around callable are deprecated. Remove the parentheses}
             edit.focus;
-//          ^error{Member function must be called}
+//          ^error{'edit.focus' must be called. Did you forgot the '\(\)'\?}
         }
     }
     x: edit.focus;
 //    ^error{Cannot convert function\(element ref\) -> void to length}
-//     ^^error{Member function must be called}
+//     ^^error{'edit.focus' must be called. Did you forgot the '\(\)'\?}
 }
diff --git a/sixtyfps_compiler/tests/syntax/lookup/callback_not_called.60 b/sixtyfps_compiler/tests/syntax/lookup/callback_not_called.60
@@ -0,0 +1,21 @@
+/* LICENSE BEGIN
+    This file is part of the SixtyFPS Project -- https://sixtyfps.io
+    Copyright (c) 2021 Olivier Goffart <olivier.goffart@sixtyfps.io>
+    Copyright (c) 2021 Simon Hausmann <simon.hausmann@sixtyfps.io>
+
+    SPDX-License-Identifier: GPL-3.0-only
+    This file is also available under commercial licensing terms.
+    Please contact info@sixtyfps.io for more information.
+LICENSE END */
+
+export Demo := Window {
+    callback foobar;
+    TouchArea {
+        clicked => {
+            root
+//          ^error{'root.foobar' must be called. Did you forgot the '\(\)'\?}
+                .foobar
+        }
+    }
+}
+
diff --git a/sixtyfps_compiler/tests/syntax/lookup/signal_arg.60 b/sixtyfps_compiler/tests/syntax/lookup/signal_arg.60
@@ -29,9 +29,12 @@ Xxx := Rectangle {
 //      ^error{The expression is not a function}
 //                ^^error{Unknown unqualified identifier 'fff'}
         (plop)("45", #fff, 42);
+//      ^warning{Parentheses around callable are deprecated. Remove the parentheses}
         (root.plop)("45", #fff, 42);
+//      ^warning{Parentheses around callable are deprecated. Remove the parentheses}
         (root.plop)("45", #fff, "45");
 //                              ^error{Cannot convert string to int}
+//      ^^warning{Parentheses around callable are deprecated. Remove the parentheses}
     }
 
     x: 12phx;