Repository: bazelbuild/bazel
Issue: Skylint should allow UpperCamelCase identifiers for provider aliases
Labels: type: bug
P2

Currently identifiers are only allowed to be `lower_snake_case` or `UPPER_SNAKE_CASE` in most cases, and also allowed to be `UpperCamelCase` if they are being [declared as a provider](https://github.com/bazelbuild/bazel/blob/901cc6c7474e2a6cc8c3812d156177f44dcb76ed/src/tools/skylark/java/com/google/devtools/skylark/skylint/NamingConventionsChecker.java#L37). This does not allow provider aliases to also be `UpperCamelCase`.

This issue came up in [this PR](https://github.com/angular/angular/pull/25256) to the angular repository. Skylint fails on [line 25](https://github.com/angular/angular/pull/25256/commits/763787afb27e8204b27b81a4e3c6c3dc6b0050b3#diff-c698d0f2da7b832a7e85568294e6653eR25) of `rules_typescript.bzl` with the following error.
```
./packages/bazel/src/rules_typescript.bzl:25:1-25:12: identifier 'TsConfigInfo' should be lower_snake_case (for variables) or UPPER_SNAKE_CASE (for constants) [name-with-wrong-case]
```

Possible solutions:

1. Always allow identifiers to be `UpperCamelCase`
2. Allow identifiers to be `UpperCamelCase` if the right side of the assignment also appears to be `UpperCamelCase`-ish.


Issue comments:
Suggestion: enforce that Identifiers are either
 * `lower_snake_case`,
 * `UPPER_SNAKE_CASE`, or
 * `UpperCamelCaseWithSuffixInfo`

Pull request review comments:
Someone told me the message could be improved:

> I think this is confusing, made me think of "information about a suffix", not of the literal suffix "Info".
> How about "UpperCamelCase and end in \"Info\" (for providers; example: FooBarInfo)" ?

Patch that resolved the issue:
diff --git a/site/docs/skylark/skylint.md b/site/docs/skylark/skylint.md
index 686ad5cd752cf9..14a282bf901e1c 100644
--- a/site/docs/skylark/skylint.md
+++ b/site/docs/skylark/skylint.md
@@ -188,9 +188,8 @@ In detail, the rules are the following:
     *   **Constants** that are **immutable**. The variable must not be rebound
         and also its deep contents must not be changed.
 *   Use **`UpperCamelCase`** for:
-    *   **Providers**. Currently, the analyzer only allows assignments of the
-        form `FooBarInfo = provider(...)`. In addition provider names have to
-        end in the **suffix `Info`**.
+    *   **Providers**. In addition, provider names have to end in the
+        **suffix `Info`**.
 *   **Never** use:
     *   **Builtin names** like `print`, `True`. Rebinding these is too
         confusing.
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/NamingConventionsChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/NamingConventionsChecker.java
index cca66fc09668b8..3531d21cd24d0d 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/NamingConventionsChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/NamingConventionsChecker.java
@@ -85,15 +85,16 @@ public void visit(AssignmentStatement node) {
     super.visit(node);
   }
 
-  private void checkSnakeCase(String name, Location location) {
-    if (!isSnakeCase(name)) {
+  private void checkIdentifier(String name, Location location) {
+    if (!isSnakeCase(name) && !(isUpperCamelCase(name) && name.endsWith("Info"))) {
       issues.add(
           Issue.create(
               NAME_WITH_WRONG_CASE_CATEGORY,
               "identifier '"
                   + name
                   + "' should be lower_snake_case (for variables)"
-                  + " or UPPER_SNAKE_CASE (for constants)",
+                  + " or UPPER_SNAKE_CASE (for constants)"
+                  + " or UpperCamelCase and end in \"Info\" (for providers; example: FizzBuzzInfo)",
               location));
     }
   }
@@ -175,7 +176,7 @@ void declare(String name, ASTNode node) {
     if (nameInfo.kind == Kind.PARAMETER || nameInfo.kind == Kind.FUNCTION) {
       checkLowerSnakeCase(nameInfo.name, node.getLocation());
     } else {
-      checkSnakeCase(nameInfo.name, node.getLocation());
+      checkIdentifier(nameInfo.name, node.getLocation());
     }
   }
 
diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NamingConventionsCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NamingConventionsCheckerTest.java
index 325e7adc07688a..cec3a81965d54c 100644
--- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NamingConventionsCheckerTest.java
+++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NamingConventionsCheckerTest.java
@@ -40,13 +40,22 @@ public void testBadLetterCase() throws Exception {
     String errorMessage =
         findIssues(
                 "badGlobalVariableName = 0",
+                "BadProviderWithoutInfoSuffix = _BadProviderWithoutInfoSuffix",
                 "def BAD_FUNCTION_NAME(BadParameterName):",
                 "  badLocalVariableName = 1")
             .toString();
     Truth.assertThat(errorMessage)
         .contains(
             "'badGlobalVariableName' should be lower_snake_case (for variables)"
-                + " or UPPER_SNAKE_CASE (for constants) [name-with-wrong-case]");
+                + " or UPPER_SNAKE_CASE (for constants)"
+                + " or UpperCamelCase and end in \"Info\" (for providers; example: FizzBuzzInfo)"
+                + " [name-with-wrong-case]");
+    Truth.assertThat(errorMessage)
+        .contains(
+            "'BadProviderWithoutInfoSuffix' should be lower_snake_case (for variables)"
+                + " or UPPER_SNAKE_CASE (for constants)"
+                + " or UpperCamelCase and end in \"Info\" (for providers; example: FizzBuzzInfo)"
+                + " [name-with-wrong-case]");
     Truth.assertThat(errorMessage)
         .contains("'BAD_FUNCTION_NAME' should be lower_snake_case [name-with-wrong-case]");
     Truth.assertThat(errorMessage)
@@ -54,7 +63,9 @@ public void testBadLetterCase() throws Exception {
     Truth.assertThat(errorMessage)
         .contains(
             "'badLocalVariableName' should be lower_snake_case (for variables)"
-                + " or UPPER_SNAKE_CASE (for constants) [name-with-wrong-case]");
+                + " or UPPER_SNAKE_CASE (for constants)"
+                + " or UpperCamelCase and end in \"Info\" (for providers; example: FizzBuzzInfo)"
+                + " [name-with-wrong-case]");
   }
 
   @Test
@@ -99,6 +110,7 @@ public void testNoIssues() throws Exception {
     Truth.assertThat(
             findIssues(
                 "GOOD_GLOBAL_VARIABLE_NAME = 0",
+                "GoodAliasedProviderInfo = _GoodAliasedProviderInfo",
                 "def good_function_name(good_parameter_name):",
                 "  GOOD_LOCAL_CONSTANT_NAME = 1"))
         .isEmpty();


Test patch: