Bug 1812011 - Add check for missing variable comments, r=gregtatum,eemeli

Differential Revision: https://phabricator.services.mozilla.com/D167787

tools/lint/fluent-lint/__init__.py

@@ -7,7 +7,7 @@ from html.parser import HTMLParser
 
 import mozpack.path as mozpath
 import yaml
-from fluent.syntax import parse, visitor
+from fluent.syntax import ast, parse, visitor
 from mozlint import result
 from mozlint.pathutils import expand_exclusions
 
@@ -64,6 +64,12 @@ class Linter(visitor.Visitor):
             # Group comments must be followed by a message. Two group comments are not
             # allowed in a row.
             "can_have_group_comment": True,
+            # Comment bound to the current message
+            "comment": "",
+            # The current group comment
+            "group_comment": "",
+            # Variables in the current message
+            "variables": [],
         }
 
         # Set this to true to debug print the root node's json. This is useful for
@@ -108,6 +114,24 @@ class Linter(visitor.Visitor):
 
         super().generic_visit(node)
 
+        # Check if variables are referenced in comments
+        if self.state["variables"]:
+            comments = self.state["comment"] + self.state["group_comment"]
+            missing_references = [
+                v for v in self.state["variables"] if f"${v}" not in comments
+            ]
+            if missing_references:
+                self.add_error(
+                    node,
+                    "VC01",
+                    "Messages including variables should have a comment "
+                    "explaining what will replace the variable. "
+                    "Missing references: "
+                    + ", ".join([f"${m}" for m in missing_references]),
+                )
+        self.state["comment"] = ""
+        self.state["variables"] = []
+
     def visit_Term(self, node):
         # There must be at least one message or term between group comments.
         self.state["can_have_group_comment"] = True
@@ -240,16 +264,33 @@ class Linter(visitor.Visitor):
         for variant in node.variants:
             super().generic_visit(variant.value)
 
+        # Store the variable used for the SelectExpression, excluding functions
+        # like PLATFORM()
+        if (
+            type(node.selector) == ast.VariableReference
+            and node.selector.id.name not in self.state["variables"]
+        ):
+            self.state["variables"].append(node.selector.id.name)
+
+    def visit_Comment(self, node):
+        # This node is a comment with: "#"
+
+        # Store the comment
+        self.state["comment"] = node.content
+
     def visit_GroupComment(self, node):
         # This node is a comment with: "##"
 
+        # Store the group comment
+        self.state["group_comment"] = node.content
+
         if not self.state["can_have_group_comment"]:
             self.add_error(
                 node,
                 "GC04",
                 "Group comments (##) must be followed by at least one message "
                 "or term. Make sure that a single group comment with multiple "
-                "pararaphs is not separated by whitespace, as it will be "
+                "paragraphs is not separated by whitespace, as it will be "
                 "interpreted as two different comments.",
             )
             return
@@ -292,9 +333,11 @@ class Linter(visitor.Visitor):
             return
 
     def visit_VariableReference(self, node):
-        # We don't recurse into variable references, the identifiers there are
-        # allowed to be free form.
-        pass
+        # Identifiers are allowed to be free form, but need to store them
+        # for comment checks.
+
+        if node.id.name not in self.state["variables"]:
+            self.state["variables"].append(node.id.name)
 
     def add_error(self, node, rule, msg):
         (col, line) = self.span_to_line_and_col(node.span)

tools/lint/test/files/fluent-lint/comment-variables1.ftl

@@ -0,0 +1,60 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+### Test group comments.
+### $var doesn't count towards commented placeables
+
+message-without-comment = This string has a { $var }
+
+## Variables:
+## $foo-group (String): group level comment
+
+# Variables:
+# $foo1 (String): just text
+message-with-comment = This string has a { $foo1 }
+
+message-with-group-comment = This string has a { $foo-group }
+
+select-without-comment1 = {
+    $select1 ->
+        [t] Foo
+       *[s] Bar
+}
+
+select-without-comment2 = {
+    $select2 ->
+        [t] Foo { $select2 }
+       *[s] Bar
+}
+
+## Variables:
+## $select4 (Integer): a number
+
+# Variables:
+# $select3 (Integer): a number
+select-with-comment1 = {
+    $select3 ->
+        [t] Foo
+       *[s] Bar
+}
+
+select-with-group-comment1 = {
+    $select4 ->
+        [t] Foo { $select4 }
+       *[s] Bar
+}
+
+message-attribute-without-comment =
+    .label = This string as { $attr }
+
+# Variables:
+# $attr2 (String): just text
+message-attribute-with-comment =
+    .label = This string as { $attr2 }
+
+message-selection-function =
+  { PLATFORM() ->
+      [macos] foo
+     *[other] bar
+  }

tools/lint/test/test_fluent_lint.py

@@ -16,7 +16,7 @@ def test_lint_exclusions(lint, paths):
 
 def test_lint_single_file(lint, paths):
     results = lint(paths("bad.ftl"))
-    assert len(results) == 11
+    assert len(results) == 13
     assert results[0].rule == "ID01"
     assert results[0].lineno == 1
     assert results[0].column == 1
@@ -50,6 +50,10 @@ def test_lint_single_file(lint, paths):
     assert results[10].rule == "ID02"
     assert results[10].lineno == 32
     assert results[10].column == 1
+    assert results[11].rule == "VC01"
+    assert "$tabCount" in results[11].message
+    assert results[12].rule == "VC01"
+    assert "$url" in results[12].message
 
 
 def test_comment_group(lint, paths):
@@ -130,5 +134,18 @@ def test_brand_names(lint, paths):
     assert len(results) == 0
 
 
+def test_comment_variables(lint, paths):
+    results = lint(paths("comment-variables1.ftl"))
+    assert len(results) == 4
+    assert results[0].rule == "VC01"
+    assert "$var" in results[0].message
+    assert results[1].rule == "VC01"
+    assert "$select1" in results[1].message
+    assert results[2].rule == "VC01"
+    assert "$select2" in results[2].message
+    assert results[3].rule == "VC01"
+    assert "$attr" in results[3].message
+
+
 if __name__ == "__main__":
     mozunit.main()