From 4aa48f9d83ae3c34ceebb172dc4479607cc54fbc Mon Sep 17 00:00:00 2001 From: vladmos Date: Wed, 12 Jun 2019 09:01:41 -0700 Subject: [PATCH] Remove Skylint Skylint has been deprecated in favor of Buildifier and is no longer maintained. PiperOrigin-RevId: 252834052 --- site/docs/skylark/skylint.md | 375 ------------- src/BUILD | 2 - src/test/starlark/BUILD | 2 +- src/test/starlark/skylint/BUILD | 18 - src/test/starlark/skylint/skylint_test.py | 111 ---- .../starlark/skylint/testdata/bad.bzl.test | 15 - .../starlark/skylint/testdata/good.bzl.test | 15 - src/test/starlark/skylint/testenv.py | 17 - .../skylint/AstVisitorWithNameResolution.java | 223 -------- .../com/google/devtools/skylark/skylint/BUILD | 38 -- .../skylark/skylint/BadOperationChecker.java | 179 ------ .../skylark/skylint/ControlFlowChecker.java | 253 --------- .../skylark/skylint/DependencyAnalyzer.java | 216 -------- .../skylark/skylint/DeprecatedApiChecker.java | 179 ------ .../skylark/skylint/DeprecationChecker.java | 158 ------ .../skylark/skylint/DocstringChecker.java | 268 --------- .../devtools/skylark/skylint/Environment.java | 221 -------- .../devtools/skylark/skylint/Issue.java | 48 -- .../devtools/skylark/skylint/Linter.java | 193 ------- .../skylark/skylint/LoadStatementChecker.java | 51 -- .../skylint/NamingConventionsChecker.java | 201 ------- .../skylint/NativeRecursiveGlobChecker.java | 132 ----- .../devtools/skylark/skylint/Skylint.java | 85 --- .../StatementWithoutEffectChecker.java | 96 ---- .../skylark/skylint/UsageChecker.java | 328 ----------- .../com/google/devtools/skylark/skylint/BUILD | 25 - .../skylint/BadOperationCheckerTest.java | 160 ------ .../skylint/ControlFlowCheckerTest.java | 348 ------------ .../skylint/DependencyAnalyzerTest.java | 237 -------- .../skylint/DeprecatedApiCheckerTest.java | 82 --- .../skylint/DeprecationCheckerTest.java | 226 -------- .../skylark/skylint/DocstringCheckerTest.java | 290 ---------- .../skylark/skylint/DocstringUtilsTest.java | 520 ------------------ .../devtools/skylark/skylint/LinterTest.java | 66 --- .../skylint/LoadStatementCheckerTest.java | 76 --- .../skylint/NamingConventionsCheckerTest.java | 138 ----- .../NativeRecursiveGlobCheckerTest.java | 91 --- .../skylark/skylint/SkylintTests.java | 22 - .../StatementWithoutEffectCheckerTest.java | 97 ---- .../skylark/skylint/UsageCheckerTest.java | 362 ------------ 40 files changed, 1 insertion(+), 6163 deletions(-) delete mode 100644 site/docs/skylark/skylint.md delete mode 100644 src/test/starlark/skylint/BUILD delete mode 100644 src/test/starlark/skylint/skylint_test.py delete mode 100644 src/test/starlark/skylint/testdata/bad.bzl.test delete mode 100644 src/test/starlark/skylint/testdata/good.bzl.test delete mode 100644 src/test/starlark/skylint/testenv.py delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/AstVisitorWithNameResolution.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/BUILD delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/DependencyAnalyzer.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecatedApiChecker.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringChecker.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/Environment.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/Issue.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/LoadStatementChecker.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/NamingConventionsChecker.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/NativeRecursiveGlobChecker.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/Skylint.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/StatementWithoutEffectChecker.java delete mode 100644 src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BUILD delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DependencyAnalyzerTest.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecatedApiCheckerTest.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTest.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringUtilsTest.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LoadStatementCheckerTest.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NamingConventionsCheckerTest.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NativeRecursiveGlobCheckerTest.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/SkylintTests.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/StatementWithoutEffectCheckerTest.java delete mode 100644 src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java diff --git a/site/docs/skylark/skylint.md b/site/docs/skylark/skylint.md deleted file mode 100644 index 0cc3cee2710403..00000000000000 --- a/site/docs/skylark/skylint.md +++ /dev/null @@ -1,375 +0,0 @@ ---- -layout: documentation -title: Skylint ---- - -# Skylint - - --> - -Skylint is deprecated in favor of [Buildifier](https://github.com/bazelbuild/buildtools/tree/master/buildifier#linter) - -This document explains how to use Skylint, a Starlark linter. - - - -## The linter CLI tool - -### Building the linter - -To build the linter from source, use the following commands: - -``` -$ git clone https://github.com/bazelbuild/bazel.git -$ cd bazel -$ bazel build //src/tools/skylark/java/com/google/devtools/skylark/skylint:Skylint -``` - -After that, the linter is available at `bazel-bin/src/tools/skylark/java/com/google/devtools/skylark/skylint/Skylint`. -You can copy it to somewhere else if you prefer. - -### Running the linter - -Assuming you have the linter binary available at `/path/to/Skylint` from the -previous step, you can run it like this: - -``` -$ /path/to/Skylint /path/to/bzl/file.bzl -``` - -where `/path/to/Skylint` is the path of the binary from the previous section. - -## The checks in detail - -This section explains which checks the linter performs and how to deal with -false positives. - -### Deprecating functions (docstring format) [deprecated-symbol] - - -To deprecate a function, add a `Deprecated:` section to the docstring, similarly -to a `Returns:` section. - -``` -def foo(): - """An example function. - - Deprecated: - - """ - # … - -def bar(): - foo() # warning: "usage of 'foo' is deprecated: " -``` - -Note that the explanation starts on the next line after `Deprecated:` and may -occupy multiple lines, with all lines indented by two spaces. - -### Using the operator + on dictionaries [deprecated-plus-dict] - - -The `+` operator (and similarly `+=`) is deprecated for dictionaries. Instead, -use the following: - -You can import a helper function from [Skylib](https://github.com/bazelbuild/bazel-skylib) -and use it like this: - -``` -load("@bazel_skylib//lib:dicts.bzl", "dicts") -dicts.add(d1, d2, d3) # instead of d1 + d2 + d3 -``` - -### Using the operator + on depset [deprecated-plus-depset] - - -The `+` operator (and similarly `+=`) is deprecated for depsets. Instead, -use the depset constructor. - -See [documentation on depsets](depsets.md) for background and examples of use. - -``` - d1 = depset(items1) - d2 = depset(items2) - combined = depset(transitive=[d1, d2]) -``` - - -### Docstrings - - - - -Categories: [missing-module-docstring] [missing-function-docstring] [bad-docstring-format] - -The Starlark conventions for docstrings are similar to the [the Python -conventions](https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments). -Docstrings are triple-quoted string literals and the closing quotes have to -appear on their own line unless the docstring is only one line long. - -A file-level docstring is the first statement of a file, even before the -`load()` statements. A function docstring is the first statement in the function -body. - -Sections in the docstring (such as `Args:` and `Returns:`) are separated by a -blank line. Their contents are indented by two spaces. -Example: - -``` -"""This module contains some docstrings examples.""" - -def example_function1(): - """Illustrates the usage of a one-line function docstring.""" - -def example_function2(foo, bar): - """Illustrates the format of a longer function docstring. - - After the one-line summary comes the description body. - It contains additional information about the function. - - The description can span more than one paragraph. - - Args: - foo: documentation of the first parameter. - Subsequent lines have to be indented by two spaces. - bar: documentation of the second parameter - - Returns: - documentation of the return value - - Deprecated: - This function is deprecated for . Use instead. - """ - return "baz" -``` - -#### Indentation - -If the linter gives you confusing error messages, **check the indentation of the -docstring**. (The indentation rules are the same as [for -Python](https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments).) -The analyzer tries to warn you about bad indentation but it may not catch all -edge cases. If the indentation is wrong, the analyzer may not recognize some -sections, such as `Args:`, which may then lead to further errors about missing -documentation for the parameters. - -#### What to document [inconsistent-docstring] - - -The analyzer **requires docstrings** for: - -* each **module** (.bzl file) -* each **public function** (i.e. a function not starting with an underscore) - that contains at least 5 statements - -If a **function has a multi-line docstring**, you also have to document (in -order): - -* all **parameters** of the function (in declaration order) -* the **return value** if the function returns a value, i.e. it contains - `return foo` instead of just `return` -* a **deprecation warning** if the function is deprecated, cf. the - [deprecation section](#deprecated-symbol) above - -### Naming conventions - - - - -Categories: [name-with-wrong-case] [provider-name-suffix] [confusing-name] - -**TL;DR**: Most Starlark identifiers should be `snake_case`, not `camelCase`. - -In detail, the rules are the following: - -* Use **`lower_snake_case`** for: - * **Functions** - * **Parameters** - * **Mutable** variables -* Use **`UPPER_SNAKE_CASE`** for: - * **Constants** that are **immutable**. The variable must not be rebound - and also its deep contents must not be changed. -* Use **`UpperCamelCase`** for: - * **Providers**. In addition, provider names have to end in the - **suffix `Info`**. -* **Never** use: - * **Builtin names** like `print`, `True`. Rebinding these is too - confusing. - * **One-letter** names that are easy to confuse, namely **`O`, `l`, `I`** - (easy to confuse with `0`, `I`, `l`, respectively). - * **Multiple underscores**: `__`, `___`, `____`, etc. -* Use the **underscore `_`**: - * **only** to ignore the result of an assignment, as in `a, _ = tuple`. - * **never** to read the value of `_`, e.g. `f(_)`. - -### Statements without effects [no-effect] - - -If a statement is just an expression that is not a function call, the analyzer -warns `expression result not used`. Most likely, you forgot to do something with -that value. Examples: `1 + foo()`, `foo[bar]`. - -#### List comprehensions - -List comprehensions inside a function should be transformed to a for-loop. -This transformation is not possible at the top level because for-loops are only -allowed inside a function to encourage declarative code at the top level. -Therefore list comprehension at the top level are allowed but you should -consider whether it would be more readable to move them to a function. Example: - -``` -# This is allowed at the top level (but consider moving it to a function): -[do_something_with(foo) for foo in bar] - -def baz(): - # This is BAD inside a function: - [do_something_with(foo) for foo in bar] - # Instead, use a for loop: - for foo in bar: - do_something_with(foo) -``` - -### Return value lint [missing-return-value] - - -If a function returns with a value (`return foo`) in some execution paths and -without one (just `return` or reaching the end of a function) in other execution -paths, the analyzer will warn about this. The reason for this is that you -probably forgot to return the right value in some execution paths. If this is -not the case, you should make your intent clear by writing `return None` -instead. - -#### Example - -``` -def foo(): - if ...: - return False - elif ...: - # do stuff - # forgot return statement here, which generates the warning - else: - return True -``` - -#### "I know the else-branch cannot happen" - -Suppose you have code like this: - -``` -def foo(): - if cond1: - return foo - elif cond2: - return bar - # I know the else-branch can't happen but the analyzer complains -``` - -In such a case, just add `fail("unreachable")` or something along these lines -to the end of the function in order to silence the warning. - -### Uninitialized variables [uninitialized-variable] - - -If a variable is not initialized before it's used on every execution path, the -analyzer warns about it: - -``` -def foo(): - if cond1: - foo = bar - elif cond2: - foo = baz - print(foo) # warning: 'foo' may not have been initialized -``` - -Most likely, the author forgot to initialize `foo` in the `else` branch. -However, if this is a false positive because you know that the `else` branch can -never happen, add an `else`-branch with the line `fail("unreachable")` or -something similar. Then the analyzer won't complain. - -If your code is more complex and it is impossible to determine statically that a -variable is initialized before usage, just initialize the variable with `None` -before using it. - -### Unused bindings [unused-binding] - - -If a binding of an identifier is not used, the analyzer warns about it: - -``` -_PRIVATE_GLOBAL_UNUSED = 0 # warns because never used -PUBLIC_GLOBAL_UNUSED = 1 # doesn't warn because it might be exported - -def public_function_unused(): # doesn't warn because it might be exported - pass - -# warns because unused function and parameter: -def _private_function_unused(param_unused): - # warns because unused local variable → rename to '_' or '_foo': - for foo in range(0, 100): - pass -``` - -The analyzer warns about unused identifiers except in the following cases: - -* If the identifier is global and public, there are no warnings because it may - be `load()`ed from somewhere else. -* If the identifier is `_`, there is no warning because the underscore signals - that the a value is ignored. -* If the identifier is a local variable and starts with an underscore, there - is no warning because the underscore signals that the value is ignored (as - opposed to global variables, where the underscore signifies that it is - private). - -#### Silencing the warning - -If you want to **silence the warning**, you can do one of the following: - -* **Remove** the unused definition. -* In case of a local variable, **rename** the variable to start with an - underscore. -* Look at the special cases below. - -#### Unused parameters with fixed names - -In case of a parameter whose name you cannot change (because you have to conform -to some API), you can use the following pattern: - -``` -def foo(param1, param2): - _ignore = (param1, param2) - # or: - _ = (param1, param2) - ... -``` - -This way, the parameters are used for the assignment of the ignored variable -`_ignore`. Hence the analyzer will not warn. - -#### Re-exporting `load()`ed names - -If you want to `load()` a name just to re-export it (and not use it in the -current file), use the following pattern. - - -``` -# If you just want to re-export 'foo', DON'T do this: -load("bar.bzl", "foo") - -# DO this instead: -load("bar.bzl", _foo="foo") -foo = _foo -``` - -This way, the name is still re-exported but doesn't generate a warning. - -### Deprecated API - -See [documentation](../skylark/lib/ctx.html) for more information. - -### Miscellaneous lints - -* **unreachable statements** [unreachable-statement] -* **Load statements** must be **at the top** of the file (after the docstring) - [load-at-top] diff --git a/src/BUILD b/src/BUILD index 24d1b18cb54b02..c0e70b803b01f2 100644 --- a/src/BUILD +++ b/src/BUILD @@ -465,8 +465,6 @@ filegroup( "//src/tools/launcher:srcs", "//src/tools/package_printer/java/com/google/devtools/build/packageprinter:srcs", "//src/tools/skylark/java/com/google/devtools/skylark/common:srcs", - "//src/tools/skylark/java/com/google/devtools/skylark/skylint:srcs", - "//src/tools/skylark/javatests/com/google/devtools/skylark/skylint:srcs", "//src/tools/xcode/realpath:srcs", "//src/tools/singlejar:srcs", "//src/tools/xcode/stdredirect:srcs", diff --git a/src/test/starlark/BUILD b/src/test/starlark/BUILD index 6694c73207287b..4172dd8ecaba35 100644 --- a/src/test/starlark/BUILD +++ b/src/test/starlark/BUILD @@ -8,7 +8,7 @@ package(default_visibility = ["//src:__subpackages__"]) filegroup( name = "srcs", - srcs = glob(["**"]) + ["//src/test/starlark/skylint:srcs"], + srcs = glob(["**"]), ) [ diff --git a/src/test/starlark/skylint/BUILD b/src/test/starlark/skylint/BUILD deleted file mode 100644 index 62d26346898fb6..00000000000000 --- a/src/test/starlark/skylint/BUILD +++ /dev/null @@ -1,18 +0,0 @@ -package(default_visibility = ["//src:__subpackages__"]) - -filegroup( - name = "srcs", - srcs = glob(["**"]), -) - -py_test( - name = "skylint_test", - srcs = [ - "skylint_test.py", - "testenv.py", - ], - data = ["//src/tools/skylark/java/com/google/devtools/skylark/skylint:Skylint"] + glob([ - "testdata/*", - ]), - python_version = "PY2", -) diff --git a/src/test/starlark/skylint/skylint_test.py b/src/test/starlark/skylint/skylint_test.py deleted file mode 100644 index 0d429dd6073183..00000000000000 --- a/src/test/starlark/skylint/skylint_test.py +++ /dev/null @@ -1,111 +0,0 @@ -# Copyright 2017 The Bazel Authors. All rights reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -import os.path -import shutil -import subprocess -import tempfile -import unittest - -from src.test.starlark.skylint import testenv - - -class SkylintTest(unittest.TestCase): - - def testGoodFile(self): - output = subprocess.check_output([ - testenv.SKYLINT_BINARY_PATH, - os.path.join(testenv.SKYLINT_TESTDATA_PATH, "good.bzl.test") - ]) - output = output.decode("utf-8") - self.assertEqual(output, "") - - def testBadFile(self): - try: - issues = "" - subprocess.check_output([ - testenv.SKYLINT_BINARY_PATH, - os.path.join(testenv.SKYLINT_TESTDATA_PATH, "bad.bzl.test") - ]) - except subprocess.CalledProcessError as e: - issues = e.output.decode("utf-8") - self.assertIn("no module docstring", issues) - - def testNonexistingFile(self): - try: - output = "" - subprocess.check_output( - [testenv.SKYLINT_BINARY_PATH, "does_not_exist.bzl"], - stderr=subprocess.STDOUT) - except subprocess.CalledProcessError as e: - output = e.output.decode("utf-8") - self.assertEqual("File not found: does_not_exist.bzl\n", output) - - def testDisablingCheck(self): - output = subprocess.check_output([ - testenv.SKYLINT_BINARY_PATH, "--disable-checks=docstring", - os.path.join(testenv.SKYLINT_TESTDATA_PATH, "bad.bzl.test") - ]) - output = output.decode("utf-8") - self.assertEqual(output, "") - - def testDisablingCategory(self): - output = subprocess.check_output([ - testenv.SKYLINT_BINARY_PATH, - "--disable-categories=missing-module-docstring", - os.path.join(testenv.SKYLINT_TESTDATA_PATH, "bad.bzl.test") - ]) - output = output.decode("utf-8") - self.assertEqual(output, "") - - IMPORT_BZL_CONTENTS = """ -def foo(): - '''bar - - Deprecated: - test.'''""" - - def GetOutputOfDependencyTestCase(self, options): - # Create these dynamically to not interfere with Bazel package structure: - temp_dir = tempfile.mkdtemp() - try: - open(os.path.join(temp_dir, "WORKSPACE"), "a").close() - open(os.path.join(temp_dir, "BUILD"), "a").close() - with open(os.path.join(temp_dir, "dependencies.bzl"), "a") as f: - f.write("'''Docstring'''\nload(':import.bzl', 'foo')\nfoo()") - with open(os.path.join(temp_dir, "import.bzl"), "a") as f: - f.write(self.IMPORT_BZL_CONTENTS) - output = None - try: - subprocess.check_output([ - testenv.SKYLINT_BINARY_PATH, - os.path.join(temp_dir, "dependencies.bzl") - ] + options) - except subprocess.CalledProcessError as e: - output = e.output.decode("utf-8") - return output - finally: - shutil.rmtree(temp_dir) - - def testDependencyAnalysis(self): - output = self.GetOutputOfDependencyTestCase([]) - self.assertIn("import.bzl) is deprecated: test.", output) - - def testSingleFileModeWorks(self): - output = self.GetOutputOfDependencyTestCase(["--single-file"]) - self.assertEqual(output, None) - - -if __name__ == "__main__": - unittest.main() diff --git a/src/test/starlark/skylint/testdata/bad.bzl.test b/src/test/starlark/skylint/testdata/bad.bzl.test deleted file mode 100644 index cfb4e2104a42e8..00000000000000 --- a/src/test/starlark/skylint/testdata/bad.bzl.test +++ /dev/null @@ -1,15 +0,0 @@ -# Copyright 2017 The Bazel Authors. All rights reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -print('Docstring missing') diff --git a/src/test/starlark/skylint/testdata/good.bzl.test b/src/test/starlark/skylint/testdata/good.bzl.test deleted file mode 100644 index b6e673792838ad..00000000000000 --- a/src/test/starlark/skylint/testdata/good.bzl.test +++ /dev/null @@ -1,15 +0,0 @@ -# Copyright 2017 The Bazel Authors. All rights reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -"""This is a docstring.""" diff --git a/src/test/starlark/skylint/testenv.py b/src/test/starlark/skylint/testenv.py deleted file mode 100644 index a5c087a8d53ebc..00000000000000 --- a/src/test/starlark/skylint/testenv.py +++ /dev/null @@ -1,17 +0,0 @@ -# Copyright 2017 The Bazel Authors. All rights reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -"""Test constants for src/test/skylark/skylint.""" - -SKYLINT_BINARY_PATH = "src/tools/skylark/java/com/google/devtools/skylark/skylint/Skylint" -SKYLINT_TESTDATA_PATH = "src/test/starlark/skylint/testdata/" diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/AstVisitorWithNameResolution.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/AstVisitorWithNameResolution.java deleted file mode 100644 index b39ba112c4ad2f..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/AstVisitorWithNameResolution.java +++ /dev/null @@ -1,223 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.base.Preconditions; -import com.google.devtools.build.lib.syntax.ASTNode; -import com.google.devtools.build.lib.syntax.AbstractComprehension; -import com.google.devtools.build.lib.syntax.AugmentedAssignmentStatement; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.DotExpression; -import com.google.devtools.build.lib.syntax.Expression; -import com.google.devtools.build.lib.syntax.FunctionDefStatement; -import com.google.devtools.build.lib.syntax.Identifier; -import com.google.devtools.build.lib.syntax.LValue; -import com.google.devtools.build.lib.syntax.ListComprehension; -import com.google.devtools.build.lib.syntax.ListLiteral; -import com.google.devtools.build.lib.syntax.LoadStatement; -import com.google.devtools.build.lib.syntax.Parameter; -import com.google.devtools.build.lib.syntax.Statement; -import com.google.devtools.build.lib.syntax.SyntaxTreeVisitor; -import java.util.Collection; - -/** - * AST visitor that keeps track of which symbols are in scope. - * - *

The methods {@code enterBlock}, {@code exitBlock}, {@code declare} and {@code reassign} can be - * overridden by a subclass to handle these "events" during AST traversal. - */ -public class AstVisitorWithNameResolution extends SyntaxTreeVisitor { - protected Environment env; - - public AstVisitorWithNameResolution() { - this(Environment.defaultBazel()); - } - - public AstVisitorWithNameResolution(Environment env) { - this.env = env; - } - - @Override - public void visit(BuildFileAST node) { - enterBlock(); - // First process all global symbols ... - for (Statement stmt : node.getStatements()) { - if (stmt instanceof FunctionDefStatement) { - Identifier fun = ((FunctionDefStatement) stmt).getIdentifier(); - env.addFunction(fun.getName(), fun); - declare(fun.getName(), fun); - } else { - visit(stmt); - } - } - // ... then check the functions - for (Statement stmt : node.getStatements()) { - if (stmt instanceof FunctionDefStatement) { - visit(stmt); - } - } - visitAll(node.getComments()); - Preconditions.checkState(env.inGlobalBlock(), "didn't exit some blocks"); - exitBlock(); - } - - @Override - public void visit(LoadStatement node) { - for (LoadStatement.Binding binding : node.getBindings()) { - String name = binding.getLocalName().getName(); - env.addImported(name, binding.getLocalName()); - declare(name, binding.getLocalName()); - } - } - - @Override - public void visit(Identifier identifier) { - use(identifier); - } - - @Override - public void visit(LValue node) { - initializeOrReassignLValue(node); - visitLvalue(node.getExpression()); - } - - protected void visitLvalue(Expression expr) { - if (expr instanceof Identifier) { - super.visit((Identifier) expr); // don't call this.visit because it doesn't count as usage - } else if (expr instanceof ListLiteral) { - for (Expression e : ((ListLiteral) expr).getElements()) { - visitLvalue(e); - } - } else { - visit(expr); - } - } - - @Override - public void visit(AugmentedAssignmentStatement node) { - for (Identifier ident : node.getLValue().boundIdentifiers()) { - use(ident); - } - super.visit(node); - } - - @Override - public void visit(FunctionDefStatement node) { - // First visit the default values for parameters in the global environment ... - for (Parameter param : node.getParameters()) { - Expression expr = param.getDefaultValue(); - if (expr != null) { - visit(expr); - } - } - // ... then visit everything else in the local environment - enterBlock(); - for (Parameter param : node.getParameters()) { - String name = param.getName(); - if (name != null) { - env.addParameter(name, param); - declare(name, param); - } - } - // The function identifier was already added to the globals before, so we skip it - visitAll(node.getParameters()); - visitBlock(node.getStatements()); - exitBlock(); - } - - @Override - public void visit(DotExpression node) { - // Don't visit the identifier field because it's not a variable and would confuse the identifier - // visitor - visit(node.getObject()); - } - - @Override - public void visit(AbstractComprehension node) { - enterBlock(); - for (ListComprehension.Clause clause : node.getClauses()) { - visit(clause.getExpression()); - LValue lvalue = clause.getLValue(); - if (lvalue != null) { - Collection boundIdents = lvalue.boundIdentifiers(); - for (Identifier ident : boundIdents) { - env.addIdentifier(ident.getName(), ident); - declare(ident.getName(), ident); - } - visit(lvalue); - } - } - visitAll(node.getOutputExpressions()); - exitBlock(); - } - - private void initializeOrReassignLValue(LValue lvalue) { - Iterable identifiers = lvalue.boundIdentifiers(); - for (Identifier identifier : identifiers) { - if (env.isDefinedInCurrentScope(identifier.getName())) { - reassign(identifier); - } else { - env.addIdentifier(identifier.getName(), identifier); - declare(identifier.getName(), identifier); - } - } - } - - /** - * Invoked when a symbol is defined during AST traversal. - * - *

This method is there to be overridden in subclasses, it doesn't do anything by itself. - * - * @param name name of the variable declared - * @param node {@code ASTNode} where it was declared - */ - void declare(String name, ASTNode node) {} - - /** - * Invoked when a variable is reassigned during AST traversal. - * - *

This method is there to be overridden in subclasses, it doesn't do anything by itself. - * - * @param ident {@code Identifier} that was reassigned - */ - void reassign(Identifier ident) {} - - /** - * Invoked when a variable is used during AST traversal. - * - *

This method is there to be overridden in subclasses, it doesn't do anything by itself. - * - * @param ident {@code Identifier} that was reassigned - */ - void use(Identifier ident) {} - - /** - * Invoked when a lexical block is entered during AST traversal. - * - *

This method is there to be overridden in subclasses. - */ - void enterBlock() { - env.enterBlock(); - } - - /** - * Invoked when a lexical block is entered during AST traversal. - * - *

This method is there to be overridden in subclasses. - */ - void exitBlock() { - env.exitBlock(); - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/BUILD b/src/tools/skylark/java/com/google/devtools/skylark/skylint/BUILD deleted file mode 100644 index baff9ba19f9846..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/BUILD +++ /dev/null @@ -1,38 +0,0 @@ -# The Skylint linter for BUILD and Skylark files -# Open-sourced as part of Bazel. - -java_binary( - name = "Skylint", - srcs = [], - main_class = "com.google.devtools.skylark.skylint.Skylint", - visibility = ["//src/test/starlark/skylint:__pkg__"], - runtime_deps = [ - ":skylint_lib", - ], -) - -java_library( - name = "skylint_lib", - srcs = glob(["**/*.java"]), - visibility = [ - # For docstring parsing libraries. - "//src/main/java/com/google/devtools/build/skydoc:__subpackages__", - "//src/tools/skylark/javatests/com/google/devtools/skylark/skylint:__pkg__", - ], - deps = [ - # TODO(bazel-team): Once BazelLibrary has a Build API interface, depend - # on lib:skylarkbuildapi instead of on lib:packages. - "//src/main/java/com/google/devtools/build/lib:packages", - "//src/main/java/com/google/devtools/build/lib:skylarkinterface", - "//src/tools/skylark/java/com/google/devtools/skylark/common", - "//third_party:guava", - ], -) - -filegroup( - name = "srcs", - srcs = glob( - ["**"], - ), - visibility = ["//src:__pkg__"], -) diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java deleted file mode 100644 index cb9460ad010a9b..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java +++ /dev/null @@ -1,179 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; -import com.google.common.collect.Maps; -import com.google.devtools.build.lib.syntax.ASTNode; -import com.google.devtools.build.lib.syntax.AssignmentStatement; -import com.google.devtools.build.lib.syntax.AugmentedAssignmentStatement; -import com.google.devtools.build.lib.syntax.BinaryOperatorExpression; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.DictComprehension; -import com.google.devtools.build.lib.syntax.DictionaryLiteral; -import com.google.devtools.build.lib.syntax.Expression; -import com.google.devtools.build.lib.syntax.FuncallExpression; -import com.google.devtools.build.lib.syntax.Identifier; -import com.google.devtools.build.lib.syntax.Operator; -import com.google.devtools.skylark.skylint.Environment.NameInfo; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; -import javax.annotation.Nullable; - -/** Checks for operations that are deprecated */ -public class BadOperationChecker extends AstVisitorWithNameResolution { - private static final String DEPRECATED_PLUS_DEPSET_CATEGORY = "deprecated-plus-depset"; - private static final String DEPRECATED_PLUS_DICT_CATEGORY = "deprecated-plus-dict"; - private static final String DEPRECATED_PIPE_CATEGORY = "deprecated-pipe-dict"; - private static final String DEPRECATED_DIVISION_CATEGORY = "deprecated-division"; - - private final List issues = new ArrayList<>(); - - enum NodeType { - DEPSET, - DICT - } - - /** - * An inferred map of variables mapped to their type. - * We consider x an 'inferred-type' variable if it appears in a statement of form `x = expr`, - * where expr is either a trivially-typed constructor (e.g. 'depset()') or is itself an - * inferred-type variable. - * - * In principle, it's possible that a variable is reassigned to a different type and that - * this map is therefore inaccurate. In practice, however, that's a fairly uncommon and - * error-prone pattern. - */ - private final Map inferredTypeVariables = Maps.newHashMap(); - - private BadOperationChecker() {} - - public static List check(BuildFileAST ast) { - BadOperationChecker checker = new BadOperationChecker(); - checker.visit(ast); - return checker.issues; - } - - /** - * If the given node is an inferred-type {@link NodeType}, return that type. - * Otherwise return null. - */ - @Nullable - private NodeType getInferredTypeOrNull(ASTNode node) { - if (node instanceof Identifier) { - NameInfo name = env.resolveName(((Identifier) node).getName()); - return name != null ? inferredTypeVariables.get(name.id) : null; - } - - if (node instanceof FuncallExpression) { - Expression function = ((FuncallExpression) node).getFunction(); - if (function instanceof Identifier) { - if (((Identifier) function).getName().equals("depset")) { - return NodeType.DEPSET; - } else if (((Identifier) function).getName().equals("dict")) { - return NodeType.DICT; - } - } - } - if (node instanceof DictionaryLiteral || node instanceof DictComprehension) { - return NodeType.DICT; - } - return null; - } - - @Override - public void visit(BinaryOperatorExpression node) { - super.visit(node); - NodeType lhsNodeType = getInferredTypeOrNull(node.getLhs()); - NodeType rhsNodeType = getInferredTypeOrNull(node.getRhs()); - - if (node.getOperator() == Operator.PLUS) { - if (lhsNodeType == NodeType.DICT || rhsNodeType == NodeType.DICT) { - issues.add( - Issue.create( - DEPRECATED_PLUS_DICT_CATEGORY, - "'+' operator is deprecated and should not be used on dictionaries", - node.getLocation())); - } - if (lhsNodeType == NodeType.DEPSET || rhsNodeType == NodeType.DEPSET) { - issues.add( - Issue.create( - DEPRECATED_PLUS_DEPSET_CATEGORY, - "'+' operator is deprecated and should not be used on depsets", - node.getLocation())); - } - } else if (node.getOperator() == Operator.PIPE) { - issues.add( - Issue.create( - DEPRECATED_PIPE_CATEGORY, - "'|' operator is deprecated and should not be used. " - + "See https://docs.bazel.build/versions/master/skylark/depsets.html " - + "for the recommended use of depsets.", - node.getLocation())); - } else if (node.getOperator() == Operator.DIVIDE) { - issues.add( - Issue.create( - DEPRECATED_DIVISION_CATEGORY, - "'/' operator is deprecated and should not be used. " - + "Use '//' instead (like in Python for floor division).", - node.getLocation())); - } - } - - @Override - public void visit(AugmentedAssignmentStatement node) { - super.visit(node); - ImmutableSet lvalues = node.getLValue().boundIdentifiers(); - if (lvalues.size() != 1) { - // assignment visitor does not track information about assignments to IndexExpressions like - // kwargs["name"] += "foo" so nothing to do here until that changes - return; - } - Identifier ident = Iterables.getOnlyElement(lvalues); - NodeType identType = getInferredTypeOrNull(ident); - NodeType expressionType = getInferredTypeOrNull(node.getExpression()); - if (identType == NodeType.DICT || expressionType == NodeType.DICT) { - issues.add( - Issue.create( - DEPRECATED_PLUS_DICT_CATEGORY, - "'+=' operator is deprecated and should not be used on dictionaries", - node.getLocation())); - } - if (identType == NodeType.DEPSET || expressionType == NodeType.DEPSET) { - issues.add( - Issue.create( - DEPRECATED_PLUS_DEPSET_CATEGORY, - "'+=' operator is deprecated and should not be used on depsets", - node.getLocation())); - } - } - - @Override - public void visit(AssignmentStatement node) { - super.visit(node); - ImmutableSet lvalues = node.getLValue().boundIdentifiers(); - if (lvalues.size() != 1) { - return; - } - Identifier ident = Iterables.getOnlyElement(lvalues); - NodeType inferredType = getInferredTypeOrNull(node.getExpression()); - - if (inferredType != null) { - inferredTypeVariables.put(env.resolveName(ident.getName()).id, inferredType); - } - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java deleted file mode 100644 index 5421193e5c709a..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java +++ /dev/null @@ -1,253 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.base.Equivalence; -import com.google.common.base.Equivalence.Wrapper; -import com.google.common.base.Preconditions; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.Expression; -import com.google.devtools.build.lib.syntax.ExpressionStatement; -import com.google.devtools.build.lib.syntax.FlowStatement; -import com.google.devtools.build.lib.syntax.ForStatement; -import com.google.devtools.build.lib.syntax.FuncallExpression; -import com.google.devtools.build.lib.syntax.FunctionDefStatement; -import com.google.devtools.build.lib.syntax.Identifier; -import com.google.devtools.build.lib.syntax.IfStatement; -import com.google.devtools.build.lib.syntax.IfStatement.ConditionalStatements; -import com.google.devtools.build.lib.syntax.ReturnStatement; -import com.google.devtools.build.lib.syntax.Statement; -import com.google.devtools.build.lib.syntax.SyntaxTreeVisitor; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.stream.Stream; -import javax.annotation.Nullable; - -/** - * Performs lints related to control flow. - * - *

For now, it only checks that if a function returns a value in some execution paths, it does so - * in all execution paths. - */ -// TODO(skylark-team): Check for unreachable statements -public class ControlFlowChecker extends SyntaxTreeVisitor { - private static final String MISSING_RETURN_VALUE_CATEGORY = "missing-return-value"; - private static final String UNREACHABLE_STATEMENT_CATEGORY = "unreachable-statement"; - private static final String NESTED_FUNCTION_CATEGORY = "nested-function"; - - private final List issues = new ArrayList<>(); - - /** - * Represents the analyzed info at the current program point. The {@code visit()} methods - * implement the transfer function from the program point immediately before that AST node to the - * program point immediately after that node. This destructively consumes (modifies) the CFI - * object, so for branching nodes a copy must be made. - * - *

See also: https://en.wikipedia.org/wiki/Data-flow_analysis - * - *

This is always null whenever we're not in a function definition. - */ - @Nullable - private ControlFlowInfo cfi = null; - - public static List check(BuildFileAST ast) { - ControlFlowChecker checker = new ControlFlowChecker(); - checker.visit(ast); - return checker.issues; - } - - @Override - public void visitBlock(List statements) { - if (cfi == null) { - super.visitBlock(statements); - return; - } - boolean alreadyReported = false; - for (Statement stmt : statements) { - if (!cfi.reachable && !alreadyReported) { - issues.add( - Issue.create( - UNREACHABLE_STATEMENT_CATEGORY, "unreachable statement", stmt.getLocation())); - alreadyReported = true; - } - visit(stmt); - } - } - - @Override - public void visit(IfStatement node) { - if (cfi == null) { - return; - } - // Save the input cfi, copy its state to seed each branch, then gather the branches together - // with a join operation to produces the output cfi. - ControlFlowInfo input = cfi; - ArrayList outputs = new ArrayList<>(); - Stream> branches = - Stream.concat( - node.getThenBlocks().stream().map(ConditionalStatements::getStatements), - Stream.of(node.getElseBlock())); - for (List branch : (Iterable>) branches::iterator) { - cfi = ControlFlowInfo.copy(input); - visitAll(branch); - outputs.add(cfi); - } - cfi = ControlFlowInfo.join(outputs); - } - - @Override - public void visit(ForStatement node) { - ControlFlowInfo noIteration = ControlFlowInfo.copy(cfi); - super.visit(node); - cfi = ControlFlowInfo.join(Arrays.asList(noIteration, cfi)); - } - - @Override - public void visit(FlowStatement node) { - Preconditions.checkNotNull(cfi); - cfi.reachable = false; - } - - @Override - public void visit(ReturnStatement node) { - // Should be rejected by parser, but we may have been fed a bad AST. - Preconditions.checkState(cfi != null, "AST has illegal top-level return statement"); - cfi.reachable = false; - cfi.returnsAlwaysExplicitly = true; - if (node.getReturnExpression() != null) { - cfi.hasReturnWithValue = true; - } else { - cfi.hasReturnWithoutValue = true; - cfi.returnStatementsWithoutValue.add(wrapReturn(node)); - } - } - - @Override - public void visit(ExpressionStatement node) { - if (cfi == null) { - return; - } - if (isFail(node.getExpression())) { - cfi.reachable = false; - cfi.returnsAlwaysExplicitly = true; - } - } - - public static boolean isFail(Expression expression) { - if (expression instanceof FuncallExpression) { - Expression function = ((FuncallExpression) expression).getFunction(); - return function instanceof Identifier && ((Identifier) function).getName().equals("fail"); - } - return false; - } - - @Override - public void visit(FunctionDefStatement node) { - if (cfi != null) { - issues.add( - Issue.create( - NESTED_FUNCTION_CATEGORY, - node.getIdentifier() - + " is a nested function which is not allowed." - + " Consider inlining it or moving it to top-level." - + " For more details, have a look at the Skylark documentation.", - node.getLocation())); - return; - } - cfi = ControlFlowInfo.entry(); - super.visit(node); - if (cfi.hasReturnWithValue && (!cfi.returnsAlwaysExplicitly || cfi.hasReturnWithoutValue)) { - issues.add( - Issue.create( - MISSING_RETURN_VALUE_CATEGORY, - "some but not all execution paths of '" - + node.getIdentifier() - + "' return a value." - + " If it is intentional, make it explicit using 'return None'." - + " If you know these cannot happen," - + " add the statement `fail('unreachable')` to them." - + " For more details, have a look at the documentation.", - node.getLocation())); - for (Wrapper returnWrapper : cfi.returnStatementsWithoutValue) { - issues.add( - Issue.create( - MISSING_RETURN_VALUE_CATEGORY, - "return value missing (you can `return None` if this is desired)", - unwrapReturn(returnWrapper).getLocation())); - } - } - cfi = null; - } - - private Wrapper wrapReturn(ReturnStatement node) { - return Equivalence.identity().wrap(node); - } - - private ReturnStatement unwrapReturn(Wrapper wrapper) { - return wrapper.get(); - } - - private static class ControlFlowInfo { - private boolean reachable; - private boolean hasReturnWithValue; - private boolean hasReturnWithoutValue; - private boolean returnsAlwaysExplicitly; - private final LinkedHashSet> returnStatementsWithoutValue; - - private ControlFlowInfo( - boolean reachable, - boolean hasReturnWithValue, - boolean hasReturnWithoutValue, - boolean returnsAlwaysExplicitly, - LinkedHashSet> returnStatementsWithoutValue) { - this.reachable = reachable; - this.hasReturnWithValue = hasReturnWithValue; - this.hasReturnWithoutValue = hasReturnWithoutValue; - this.returnsAlwaysExplicitly = returnsAlwaysExplicitly; - this.returnStatementsWithoutValue = returnStatementsWithoutValue; - } - - /** Create a CFI corresponding to an entry point in the control-flow graph. */ - static ControlFlowInfo entry() { - return new ControlFlowInfo(true, false, false, false, new LinkedHashSet<>()); - } - - /** Creates a copy of a CFI, including the {@code returnStatementsWithoutValue} collection. */ - static ControlFlowInfo copy(ControlFlowInfo existing) { - return new ControlFlowInfo( - existing.reachable, - existing.hasReturnWithValue, - existing.hasReturnWithoutValue, - existing.returnsAlwaysExplicitly, - new LinkedHashSet<>(existing.returnStatementsWithoutValue)); - } - - /** Joins the CFIs for several alternative paths together. */ - static ControlFlowInfo join(List infos) { - ControlFlowInfo result = - new ControlFlowInfo(false, false, false, true, new LinkedHashSet<>()); - for (ControlFlowInfo info : infos) { - result.reachable |= info.reachable; - result.hasReturnWithValue |= info.hasReturnWithValue; - result.hasReturnWithoutValue |= info.hasReturnWithoutValue; - result.returnsAlwaysExplicitly &= info.returnsAlwaysExplicitly; - result.returnStatementsWithoutValue.addAll(info.returnStatementsWithoutValue); - } - return result; - } - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DependencyAnalyzer.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DependencyAnalyzer.java deleted file mode 100644 index 8f2abb8bf98658..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DependencyAnalyzer.java +++ /dev/null @@ -1,216 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.LoadStatement; -import com.google.devtools.build.lib.syntax.Statement; -import com.google.devtools.skylark.skylint.Linter.FileFacade; -import java.io.IOException; -import java.nio.file.Path; -import java.util.LinkedHashMap; -import java.util.LinkedHashSet; -import java.util.Map; -import java.util.Set; -import javax.annotation.Nullable; - -/** - * Helps collect information about direct and transitive dependencies of a Skylark file. - * - * @param the type of information associated with a file - */ -public class DependencyAnalyzer { - private final FileFacade fileFacade; - private final Map pathToInfo = new LinkedHashMap<>(); - private Map cachedWorkspaceRoot = new LinkedHashMap<>(); - private Map cachedPackageRoot = new LinkedHashMap<>(); - private DependencyCollector collector; - private Set visited = new LinkedHashSet<>(); - - private static final ImmutableList BUILD_FILES = ImmutableList.of("BUILD", "BUILD.bazel"); - private static final ImmutableList WORKSPACE_FILE = ImmutableList.of("WORKSPACE"); - - /** - * Creates an instance of DependencyAnalyzer that can be reused for multiple Skylark files. - * - * @param fileFacade interface to access file contents - * @param collector extracts the desired information from a Skylark file - */ - public DependencyAnalyzer(FileFacade fileFacade, DependencyCollector collector) { - this.fileFacade = fileFacade; - this.collector = collector; - } - - /** - * Collects information about the given file and its direct and transitive dependencies. - * - *

The information is cached between calls to this method, so it won't reanalyze the same file - * twice. This applies even if a file is loaded via different path labels that correspond to the - * same canonical path. - * - * @param path the path of the file to be analyzed - * @return the information about that file, or null if it can't be read - */ - @Nullable - public T collectTransitiveInfo(Path path) { - path = path.toAbsolutePath(); - if (visited.contains(path)) { - return pathToInfo.get(path); - } - visited.add(path); - BuildFileAST ast; - try { - ast = fileFacade.readAst(path); - } catch (IOException e) { - return null; - } - T info = collector.initInfo(path); - for (Statement stmt : ast.getStatements()) { - if (stmt instanceof LoadStatement) { - String label = ((LoadStatement) stmt).getImport().getValue(); - Path dep = labelToPath(label, path); - if (dep == null) { - continue; - } - T depInfo = collectTransitiveInfo(dep); - if (depInfo == null) { - continue; // may happen if there's an illegal dependency cycle - } - info = collector.loadDependency(info, (LoadStatement) stmt, dep, depInfo); - } - } - info = collector.collectInfo(path, ast, info); - pathToInfo.put(path, info); - return info; - } - - @Nullable - private Path findAncestorDirectoryContainingAnyOf(Path path, Iterable fileNames) { - Path dir = path.toAbsolutePath(); - while ((dir = dir.getParent()) != null) { - for (String fileName : fileNames) { - if (fileFacade.fileExists(dir.resolve(fileName))) { - return dir; - } - } - } - return null; - } - - /** - * Resolves the label of a load statement to a path. - * - * @param label the import of a load statement - * @param currentPath the path of the file containing the load statement - * @return the path corresponding to the label or null if it can't be resolved - */ - @Nullable - private Path labelToPath(String label, Path currentPath) { - if (label.startsWith("@")) { - // TODO(skylark-team): analyze such dependencies as well - return null; - } else if (label.startsWith("//")) { - Path workspaceRoot = getWorkspaceRoot(currentPath); - if (workspaceRoot == null) { - return null; - } - label = label.substring(label.startsWith("//:") ? 3 : 2); - return workspaceRoot.resolve(label.replace(':', '/')); - } else if (label.startsWith(":")) { - Path packageRoot = getPackageRoot(currentPath); - if (packageRoot == null) { - return null; - } - return packageRoot.resolve(label.substring(1)); - } else { - // otherwise just treat it as a though it started with "//" - Path workspaceRoot = getWorkspaceRoot(currentPath); - if (workspaceRoot == null) { - return null; - } - return workspaceRoot.resolve(label.replace(':', '/')); - } - } - - @Nullable - private Path getPackageRoot(Path path) { - if (!cachedPackageRoot.containsKey(path)) { - cachedPackageRoot.put(path, findAncestorDirectoryContainingAnyOf(path, BUILD_FILES)); - } - return cachedPackageRoot.get(path); - } - - @Nullable - private Path getWorkspaceRoot(Path path) { - if (!cachedWorkspaceRoot.containsKey(path)) { - cachedWorkspaceRoot.put(path, findAncestorDirectoryContainingAnyOf(path, WORKSPACE_FILE)); - } - return cachedWorkspaceRoot.get(path); - } - - /** - * Encapsulates how to produce information about a Skylark file, given its contents and the - * information about its transitive dependencies. - * - *

Each of the methods returns new or updated instances of T associated with the file. - * - *

When analyzing a file, the methods will be invoked in the following order: - * - *

    - *
  1. {@link DependencyCollector#initInfo} to return an initial info object - *
  2. {@link DependencyCollector#loadDependency} is iteratively called for each direct - * dependency to get an updated info object that accounts for the info from this dependency, - * until all direct dependencies have been processed - *
  3. {@link DependencyCollector#collectInfo} is called to get a final updated info object that - * accounts for the content of the current file. - *
- * - * @param the type of information being collected - */ - public interface DependencyCollector { - - /** - * Used to initialize the dependency information when starting analysis of a file. - * - * @param path the path of the current file - * @return the initial information about the current file - */ - T initInfo(Path path); - - /** - * Incorporates the information about a dependency in the information about the file. - * - *

This method is called for every load() statement in the current file. - * - * @param currentFileInfo info about the current file so far, may be modified - * @param stmt the load statement being processed - * @param loadedPath the path of the dependency that is load()ed - * @param loadedFileInfo info about the dependency file that is load()ed, must not be modified - * @return the updated information about the current file - */ - T loadDependency(T currentFileInfo, LoadStatement stmt, Path loadedPath, T loadedFileInfo); - - /** - * Collect information about the current file after the load statements have been processed. - * - * @param path the path of the current file - * @param ast the ast of the current file - * @param info the information about the current file so far, may be modified - * @return the updated information about the current file - */ - T collectInfo(Path path, BuildFileAST ast, T info); - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecatedApiChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecatedApiChecker.java deleted file mode 100644 index 6135bd738c8b17..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecatedApiChecker.java +++ /dev/null @@ -1,179 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Sets; -import com.google.devtools.build.lib.syntax.Argument; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.DotExpression; -import com.google.devtools.build.lib.syntax.Expression; -import com.google.devtools.build.lib.syntax.FuncallExpression; -import com.google.devtools.build.lib.syntax.FunctionDefStatement; -import com.google.devtools.build.lib.syntax.Identifier; -import com.google.devtools.build.lib.syntax.ReturnStatement; -import com.google.devtools.build.lib.syntax.SyntaxTreeVisitor; -import java.util.ArrayList; -import java.util.List; -import java.util.Set; - -/** Checks for operations that are deprecated */ -public class DeprecatedApiChecker extends AstVisitorWithNameResolution { - private static final String DEPRECATED_API = "deprecated-api"; - private static final String RULE_IMPL_RETURN = "deprecated-rule-impl-return"; - - private final List issues = new ArrayList<>(); - - /** True if we are currently visiting a rule implementation. */ - private boolean visitingRuleImplementation; - - /** Set of functions that are used as rule implementation. */ - private final Set ruleImplSet = Sets.newHashSet(); - - private DeprecatedApiChecker() {} - - public static List check(BuildFileAST ast) { - DeprecatedApiChecker checker = new DeprecatedApiChecker(); - checker.inferRuleImpl(ast); - checker.visit(ast); - return checker.issues; - } - - /** - * Convert a dotted expression to string, e.g. rule -> "rule" attr.label -> "attr.label" - * - *

If input contains anything else than Identifier or DotExpression, return empty string. - */ - private static String dottedExpressionToString(Expression e) { - if (e instanceof Identifier) { - return ((Identifier) e).getName(); - } - if (e instanceof DotExpression) { - String result = dottedExpressionToString(((DotExpression) e).getObject()); - if (!result.isEmpty()) { - return result + "." + ((DotExpression) e).getField().getName(); - } - } - - return ""; - } - - private void inferRuleImpl(BuildFileAST ast) { - new SyntaxTreeVisitor() { - - @Override - public void visit(FuncallExpression node) { - // Collect all 'x' that match this pattern: - // rule(implementation=x, ...) - Expression fct = node.getFunction(); - if (!(fct instanceof Identifier) || !((Identifier) fct).getName().equals("rule")) { - return; - } - - boolean firstArg = true; - for (Argument.Passed arg : node.getArguments()) { - if (!"implementation".equals(arg.getName()) && (!firstArg || arg.isKeyword())) { - firstArg = false; - continue; - } - firstArg = false; - Expression val = arg.getValue(); - if (val instanceof Identifier) { - ruleImplSet.add(((Identifier) val).getName()); - } - } - } - }.visit(ast); - } - - private static final ImmutableMap deprecatedMethods = - ImmutableMap.builder() - .put("ctx.action", "Use ctx.actions.run or ctx.actions.run_shell.") - .put("ctx.default_provider", "Use DefaultInfo.") - .put("ctx.empty_action", "Use ctx.actions.do_nothing.") - .put("ctx.expand_make_variables", "Use ctx.var to access the variables.") - .put("ctx.file_action", "Use ctx.actions.write.") - .put("ctx.new_file", "Use ctx.actions.declare_file.") - .put("ctx.template_action", "Use ctx.actions.expand_template.") - .put("PACKAGE_NAME", "Use native.package_name().") - .put("REPOSITORY_NAME", "Use native.repository_name().") - .put("FileType", "Use a list of strings.") - .put( - "ctx.outputs.executable", - "See https://docs.bazel.build/versions/master/skylark/" - + "rules.html#executable-rules-and-test-rules") - .put( - "native.package", - "Call package() in the BUILD file instead. " - + "See https://github.com/bazelbuild/bazel/issues/5939.") - .build(); - - private void checkDeprecated(Expression node) { - String name = dottedExpressionToString(node); - if (deprecatedMethods.containsKey(name)) { - issues.add( - Issue.create( - DEPRECATED_API, - name + " is deprecated: " + deprecatedMethods.get(name), - node.getLocation())); - } - } - - @Override - public void visit(Identifier node) { - super.visit(node); - checkDeprecated(node); - } - - @Override - public void visit(DotExpression node) { - super.visit(node); - checkDeprecated(node); - } - - @Override - public void visit(ReturnStatement node) { - super.visit(node); - - // Check that rule implementation functions don't return a call to `struct`. - if (!visitingRuleImplementation) { - return; - } - Expression e = node.getReturnExpression(); - if (e == null) { - return; - } - if (!(e instanceof FuncallExpression)) { - return; - } - String fctName = dottedExpressionToString(((FuncallExpression) e).getFunction()); - if (fctName.equals("struct")) { - issues.add( - Issue.create( - RULE_IMPL_RETURN, - "Avoid using the legacy provider syntax. Instead of returning a `struct` from a rule " - + "implementation function, return a list of providers: " - + "https://docs.bazel.build/versions/master/skylark/rules.html" - + "#migrating-from-legacy-providers", - node.getLocation())); - } - } - - @Override - public void visit(FunctionDefStatement node) { - visitingRuleImplementation = ruleImplSet.contains(node.getIdentifier().getName()); - super.visit(node); - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java deleted file mode 100644 index d1dd1bfe7a86ea..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java +++ /dev/null @@ -1,158 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.FunctionDefStatement; -import com.google.devtools.build.lib.syntax.Identifier; -import com.google.devtools.build.lib.syntax.LoadStatement; -import com.google.devtools.build.lib.syntax.StringLiteral; -import com.google.devtools.skylark.common.DocstringUtils; -import com.google.devtools.skylark.common.DocstringUtils.DocstringInfo; -import com.google.devtools.skylark.skylint.DependencyAnalyzer.DependencyCollector; -import com.google.devtools.skylark.skylint.Environment.NameInfo; -import com.google.devtools.skylark.skylint.Environment.NameInfo.Kind; -import com.google.devtools.skylark.skylint.Linter.FileFacade; -import java.nio.file.Path; -import java.util.ArrayList; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; - -/** Checks for usage of deprecated symbols. */ -public class DeprecationChecker extends AstVisitorWithNameResolution { - private static final String DEPRECATED_SYMBOL_CATEGORY = "deprecated-symbol"; - - private final List issues = new ArrayList<>(); - /** Maps a global function name to its deprecation information, if any. */ - private final Map symbolToDeprecation; - /** Path of the file that is checked. */ - private final Path path; - - private DeprecationChecker(Path path, Map symbolToDeprecation) { - this.path = path; - this.symbolToDeprecation = symbolToDeprecation; - } - - public static List check(Path path, BuildFileAST ast, FileFacade fileFacade) { - Map symbolToDeprecationWarning = - DeprecationCollector.getDeprecations(path, fileFacade); - DeprecationChecker checker = new DeprecationChecker(path, symbolToDeprecationWarning); - checker.visit(ast); - return checker.issues; - } - - @Override - public void visit(FunctionDefStatement node) { - // Don't issue deprecation warnings inside of deprecated functions: - if (!symbolToDeprecation.containsKey(node.getIdentifier().getName())) { - super.visit(node); - } - } - - @Override - void use(Identifier ident) { - NameInfo info = env.resolveName(ident.getName()); - if (info == null) { - return; - } - DeprecatedSymbol deprecation = symbolToDeprecation.get(info.name); - if (deprecation != null && info.kind != Kind.LOCAL && info.kind != Kind.PARAMETER) { - String originInfo = ""; - if (deprecation.origin != path) { - originInfo = "(imported from " + deprecation.origin; - if (!deprecation.originalName.equals(info.name)) { - originInfo += ", named '" + deprecation.originalName + "' there"; - } - originInfo += ") "; - } - String message = - "usage of '" - + info.name - + "' " - + originInfo - + "is deprecated: " - + deprecation.deprecationMessage; - issues.add(Issue.create(DEPRECATED_SYMBOL_CATEGORY, message, ident.getLocation())); - } - } - - /** Holds information about a deprecated symbol. */ - private static class DeprecatedSymbol { - final Path origin; - final String originalName; - final String deprecationMessage; - - public DeprecatedSymbol(Path origin, String originalName, String deprecationMessage) { - this.origin = origin; - this.originalName = originalName; - this.deprecationMessage = deprecationMessage; - } - } - - /** Collects information about deprecated symbols (including dependencies). */ - private static class DeprecationCollector - implements DependencyCollector> { - - /** - * Returns deprecation information (including dependencies) about symbols in the given file. - * - * @param path the path of the file to collect deprecation from - * @param fileFacade to access files - * @return a map: symbol name -> deprecation info - */ - public static Map getDeprecations(Path path, FileFacade fileFacade) { - return new DependencyAnalyzer<>(fileFacade, new DeprecationCollector()) - .collectTransitiveInfo(path); - } - - @Override - public Map initInfo(Path path) { - return new LinkedHashMap<>(); - } - - @Override - public Map loadDependency( - Map currentFileInfo, - LoadStatement stmt, - Path loadedPath, - Map loadedFileInfo) { - for (LoadStatement.Binding binding : stmt.getBindings()) { - String originalName = binding.getOriginalName().getName(); - String alias = binding.getLocalName().getName(); - DeprecatedSymbol originalDeprecation = loadedFileInfo.get(originalName); - if (originalDeprecation != null) { - currentFileInfo.put(alias, originalDeprecation); - } - } - return currentFileInfo; - } - - @Override - public Map collectInfo( - Path path, BuildFileAST ast, Map deprecationInfos) { - Map docstrings = DocstringUtils.collectDocstringLiterals(ast); - for (Map.Entry entry : docstrings.entrySet()) { - String symbol = entry.getKey(); - StringLiteral docstring = entry.getValue(); - DocstringInfo info = DocstringUtils.parseDocstring(docstring, new ArrayList<>()); - if (!info.getDeprecated().isEmpty()) { - deprecationInfos.put(symbol, new DeprecatedSymbol(path, symbol, info.getDeprecated())); - } - } - return deprecationInfos; - } - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringChecker.java deleted file mode 100644 index 505938d0d29dd2..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringChecker.java +++ /dev/null @@ -1,268 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import static com.google.devtools.skylark.common.DocstringUtils.extractDocstring; - -import com.google.devtools.build.lib.events.Location.LineAndColumn; -import com.google.devtools.build.lib.syntax.ASTNode; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.Expression; -import com.google.devtools.build.lib.syntax.FunctionDefStatement; -import com.google.devtools.build.lib.syntax.Parameter; -import com.google.devtools.build.lib.syntax.ReturnStatement; -import com.google.devtools.build.lib.syntax.Statement; -import com.google.devtools.build.lib.syntax.StringLiteral; -import com.google.devtools.build.lib.syntax.SyntaxTreeVisitor; -import com.google.devtools.skylark.common.DocstringUtils; -import com.google.devtools.skylark.common.DocstringUtils.DocstringInfo; -import com.google.devtools.skylark.common.DocstringUtils.DocstringParseError; -import com.google.devtools.skylark.common.DocstringUtils.ParameterDoc; -import com.google.devtools.skylark.common.LocationRange; -import com.google.devtools.skylark.common.LocationRange.Location; -import java.util.ArrayList; -import java.util.LinkedHashSet; -import java.util.List; - -/** Checks the existence of docstrings. */ -public class DocstringChecker extends SyntaxTreeVisitor { - private static final String MISSING_MODULE_DOCSTRING_CATEGORY = "missing-module-docstring"; - private static final String MISSING_FUNCTION_DOCSTRING_CATEGORY = "missing-function-docstring"; - private static final String INCONSISTENT_DOCSTRING_CATEGORY = "inconsistent-docstring"; - private static final String BAD_DOCSTRING_FORMAT_CATEGORY = "bad-docstring-format"; - private static final String ARGS_ARGUMENTS_DOCSTRING_CATEGORY = "args-arguments-docstring"; - /** If a function is at least this many statements long, a docstring is required. */ - private static final int FUNCTION_LENGTH_DOCSTRING_THRESHOLD = 5; - - private final List issues = new ArrayList<>(); - private boolean containsReturnWithValue = false; - - public static List check(BuildFileAST ast) { - DocstringChecker checker = new DocstringChecker(); - ast.accept(checker); - return checker.issues; - } - - @Override - public void visit(BuildFileAST node) { - StringLiteral moduleDocstring = extractDocstring(node.getStatements()); - if (moduleDocstring == null) { - // The reported location starts on the first line since that's where the docstring is expected - Location start = new Location(1, 1); - // This location is invalid if the file is empty but this edge case is not worth the trouble. - Location end = new Location(2, 1); - LocationRange range = new LocationRange(start, end); - issues.add( - new Issue(MISSING_MODULE_DOCSTRING_CATEGORY, "file has no module docstring", range)); - } else { - List errors = new ArrayList<>(); - DocstringUtils.parseDocstring(moduleDocstring, errors); - for (DocstringParseError error : errors) { - issues.add(docstringParseErrorToIssue(moduleDocstring, error)); - } - } - super.visit(node); - } - - @Override - public void visit(ReturnStatement node) { - if (node.getReturnExpression() != null) { - containsReturnWithValue = true; - } - } - - @Override - public void visit(FunctionDefStatement node) { - containsReturnWithValue = false; - super.visit(node); - StringLiteral functionDocstring = extractDocstring(node.getStatements()); - if (functionDocstring == null - && !node.getIdentifier().getName().startsWith("_") - && countNestedStatements(node) >= FUNCTION_LENGTH_DOCSTRING_THRESHOLD) { - Location start = Location.from(node.getLocation().getStartLineAndColumn()); - Location end; - if (node.getStatements().isEmpty()) { - // empty statement suites cannot come from the parser yet we should handle this gracefully: - end = Location.from(node.getLocation().getEndLineAndColumn()); - } else { - LineAndColumn lac = node.getStatements().get(0).getLocation().getStartLineAndColumn(); - end = new Location(lac.getLine(), lac.getColumn() - 1); // right before the first statement - } - String name = node.getIdentifier().getName(); - issues.add( - new Issue( - MISSING_FUNCTION_DOCSTRING_CATEGORY, - "function '" - + name - + "' has no docstring" - + " (if this function is intended to be private," - + " the name should start with an underscore: '_" - + name - + "')", - new LocationRange(start, end))); - } - if (functionDocstring == null) { - return; - } - List errors = new ArrayList<>(); - DocstringInfo info = DocstringUtils.parseDocstring(functionDocstring, errors); - for (DocstringParseError error : errors) { - issues.add(docstringParseErrorToIssue(functionDocstring, error)); - } - if (!info.isSingleLineDocstring()) { - checkMultilineFunctionDocstring( - node, functionDocstring, info, containsReturnWithValue, issues); - } - if (info.getArgumentsLocation() != null) { - int lineOffset = functionDocstring.getLocation().getStartLine() - 1; - issues.add( - new Issue( - ARGS_ARGUMENTS_DOCSTRING_CATEGORY, - "Prefer 'Args:' to 'Arguments:' when documenting function arguments.", - new LocationRange( - new Location( - info.getArgumentsLocation().start.line + lineOffset, - info.getArgumentsLocation().start.column), - new Location( - info.getArgumentsLocation().end.line + lineOffset, - info.getArgumentsLocation().end.column)))); - } - } - - private static class StatementCounter extends SyntaxTreeVisitor { - public int count = 0; - - @Override - public void visitBlock(List statements) { - count += statements.size(); - } - } - - private static int countNestedStatements(ASTNode node) { - StatementCounter counter = new StatementCounter(); - counter.visit(node); - return counter.count; - } - - private static void checkMultilineFunctionDocstring( - FunctionDefStatement functionDef, - StringLiteral docstringLiteral, - DocstringInfo docstring, - boolean functionReturnsWithValue, - List issues) { - if (functionReturnsWithValue && docstring.getReturns().isEmpty()) { - issues.add( - Issue.create( - INCONSISTENT_DOCSTRING_CATEGORY, - "incomplete docstring: the return value is not documented" - + " (no 'Returns:' section found)", - docstringLiteral.getLocation())); - } - List documentedParams = new ArrayList<>(); - for (ParameterDoc param : docstring.getParameters()) { - documentedParams.add(param.getParameterName()); - } - List declaredParams = new ArrayList<>(); - for (Parameter param : functionDef.getParameters()) { - if (param.getName() != null) { - String name = param.getName(); - if (param.isStar()) { - name = "*" + name; - } - if (param.isStarStar()) { - name = "**" + name; - } - declaredParams.add(name); - } - } - checkParamListsMatch(docstringLiteral, documentedParams, declaredParams, issues); - } - - private static void checkParamListsMatch( - StringLiteral docstringLiteral, - List documentedParams, - List declaredParams, - List issues) { - if (documentedParams.isEmpty() && !declaredParams.isEmpty()) { - StringBuilder message = - new StringBuilder("incomplete docstring: the function parameters are not documented") - .append(" (no 'Args:' section found)\n") - .append("The parameter documentation should look like this:\n\n") - .append("Args:\n"); - for (String param : declaredParams) { - message.append(" ").append(param).append(": ...\n"); - } - message.append("\n"); - issues.add( - Issue.create( - INCONSISTENT_DOCSTRING_CATEGORY, message.toString(), docstringLiteral.getLocation())); - return; - } - for (String param : declaredParams) { - if (!documentedParams.contains(param)) { - issues.add( - Issue.create( - INCONSISTENT_DOCSTRING_CATEGORY, - "incomplete docstring: parameter '" + param + "' not documented", - docstringLiteral.getLocation())); - } - } - for (String param : documentedParams) { - if (!declaredParams.contains(param)) { - issues.add( - Issue.create( - INCONSISTENT_DOCSTRING_CATEGORY, - "inconsistent docstring: parameter '" - + param - + "' appears in docstring but not in function signature", - docstringLiteral.getLocation())); - } - } - if (new LinkedHashSet<>(declaredParams).equals(new LinkedHashSet<>(documentedParams)) - && !declaredParams.equals(documentedParams)) { - String message = - "inconsistent docstring: order of parameters differs from function signature\n" - + "Declaration order: " - + String.join(", ", declaredParams) - + "\n" - + "Documentation order: " - + String.join(", ", documentedParams) - + "\n"; - issues.add( - Issue.create(INCONSISTENT_DOCSTRING_CATEGORY, message, docstringLiteral.getLocation())); - } - } - - private Issue docstringParseErrorToIssue(StringLiteral docstring, DocstringParseError error) { - int startLine = docstring.getLocation().getStartLine() + error.getLineNumber() - 1; - int startColumn; - if (error.getLineNumber() == 1) { - // The Skylark AST does not expose whether the string literal was a triple-quoted string, so - // we just assume the most common case: triple-quoted docstrings. - // There's also the possibility of a raw string (r'''docstring'''), in which case we would - // have to add 4 to the column instead of 3. - // TODO(skylark-team): Clean this up once the AST contains more information. - startColumn = docstring.getLocation().getStartLineAndColumn().getColumn() + 3; - } else { - startColumn = 1; - } - Location start = new Location(startLine, startColumn); - Location end = new Location(startLine, Math.max(1, startColumn + error.getLine().length() - 1)); - return new Issue( - BAD_DOCSTRING_FORMAT_CATEGORY, - "bad docstring format: " + error.getMessage(), - new LocationRange(start, end)); - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Environment.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Environment.java deleted file mode 100644 index f0ae3e7511fa5f..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Environment.java +++ /dev/null @@ -1,221 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.packages.BazelLibrary; -import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature; -import com.google.devtools.build.lib.syntax.ASTNode; -import com.google.devtools.build.lib.syntax.Comment; -import com.google.devtools.build.lib.syntax.Identifier; -import com.google.devtools.build.lib.syntax.MethodLibrary; -import com.google.devtools.skylark.skylint.Environment.NameInfo.Kind; -import java.lang.reflect.Field; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import javax.annotation.Nullable; - -/** Holds the information about which symbols are in scope during AST traversal. */ -public class Environment { - private final List blocks = new ArrayList<>(); - private final Map idToNameInfo = new HashMap<>(); - private int nextId = 0; - private static final int BUILTINS_INDEX = 0; // index of the block containing builtins - private static final int GLOBALS_INDEX = 1; // index of the block containing globals - - public static Environment empty() { - Environment env = new Environment(); - env.blocks.add(new LexicalBlock()); // for builtins - return env; - } - - public static Environment defaultBazel() { - Environment env = empty(); - env.addBuiltin("None"); - env.addBuiltin("True"); - env.addBuiltin("False"); - env.setupFunctions(BazelLibrary.class); - ImmutableMap.Builder builtinMap = ImmutableMap.builder(); - MethodLibrary.addBindingsToBuilder(builtinMap); - for (String builtinName : builtinMap.build().keySet()) { - env.addBuiltin(builtinName); - } - return env; - } - - private void setupFunctions(Class... classes) { - // Iterate through the skylark functions declared inline within the classes - // retrieving and storing their names. - for (Class c : classes) { - for (Field field : c.getDeclaredFields()) { - // Skylark functions are defined inline as fields within the class, annotated - // by @SkylarkSignature. - SkylarkSignature builtinFuncSignature = field.getAnnotation(SkylarkSignature.class); - if (builtinFuncSignature != null && builtinFuncSignature.objectType() == Object.class) { - addBuiltin(builtinFuncSignature.name()); - } - } - } - } - - public void enterBlock() { - blocks.add(new LexicalBlock()); - } - - public Collection exitBlock() { - if (blocks.size() <= GLOBALS_INDEX) { - throw new IllegalStateException("no blocks to exit from"); - } - return blocks.remove(blocks.size() - 1).getAllSymbols(); - } - - public boolean inGlobalBlock() { - return blocks.size() - 1 == GLOBALS_INDEX; - } - - public boolean isDefined(String name) { - return resolveName(name) != null; - } - - public boolean isDefinedInCurrentScope(String name) { - return blocks.get(blocks.size() - 1).resolve(name) != null; - } - - @Nullable - public NameInfo resolveName(String name) { - for (int i = blocks.size() - 1; i >= 0; i--) { - Integer id = blocks.get(i).resolve(name); - if (id != null) { - return idToNameInfo.get(id); - } - } - return null; - } - - public NameInfo resolveExistingName(String name) { - NameInfo info = resolveName(name); - if (info == null) { - throw new IllegalArgumentException("name '" + name + "' doesn't exist"); - } - return info; - } - - private void addName(int block, NameInfo nameInfo) { - NameInfo prev = idToNameInfo.putIfAbsent(nameInfo.id, nameInfo); - if (prev != null) { - throw new IllegalStateException("id " + nameInfo.id + " is already used!"); - } - blocks.get(block).add(nameInfo.name, nameInfo.id); - } - - private void addBuiltin(String name) { - addName(BUILTINS_INDEX, createNameInfo(name, new Comment("builtin"), Kind.BUILTIN)); - } - - public void addImported(String name, Identifier node) { - addName(GLOBALS_INDEX, createNameInfo(name, node, Kind.IMPORTED)); - } - - public void addIdentifier(String name, ASTNode node) { - Kind kind = blocks.size() - 1 == GLOBALS_INDEX ? Kind.GLOBAL : Kind.LOCAL; - addName(blocks.size() - 1, createNameInfo(name, node, kind)); - } - - public void addFunction(String name, ASTNode node) { - addName(GLOBALS_INDEX, createNameInfo(name, node, Kind.FUNCTION)); - } - - public void addParameter(String name, ASTNode param) { - addName(blocks.size() - 1, createNameInfo(name, param, Kind.PARAMETER)); - } - - private NameInfo createNameInfo(String name, ASTNode definition, Kind kind) { - return new NameInfo(name, definition, newId(), kind); - } - - private int newId() { - int ret = nextId; - nextId++; - return ret; - } - - public Collection getNameIdsInCurrentBlock() { - return getNameIdsInBlock(blocks.size() - 1); - } - - private Collection getNameIdsInBlock(int block) { - return Collections.unmodifiableCollection(blocks.get(block).nameToId.values()); - } - - public NameInfo getNameInfo(int id) { - NameInfo info = idToNameInfo.get(id); - if (info == null) { - throw new IllegalArgumentException("id " + id + " doesn't exist"); - } - return info; - } - - /** - * Represents a lexical block, e.g. global, function-local or further nested (in a comprehension). - */ - private static class LexicalBlock { - private final Map nameToId = new HashMap<>(); - - @Nullable - private Integer resolve(String name) { - return nameToId.get(name); - } - - private void add(String name, int id) { - Integer entry = nameToId.putIfAbsent(name, id); - if (entry != null) { - throw new IllegalArgumentException("name '" + name + "' is already defined"); - } - } - - public Collection getAllSymbols() { - return nameToId.values(); - } - } - - /** Holds information about a name/symbol. */ - public static class NameInfo { - final int id; - final String name; - final ASTNode definition; - final Kind kind; - - /** Kind of definition where the name was declared. */ - public enum Kind { - BUILTIN, - IMPORTED, - GLOBAL, - FUNCTION, - PARAMETER, - LOCAL, - } - - private NameInfo(String name, ASTNode definition, int id, Kind kind) { - this.id = id; - this.name = name; - this.definition = definition; - this.kind = kind; - } - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Issue.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Issue.java deleted file mode 100644 index 55d26f9435ec8b..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Issue.java +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.devtools.build.lib.events.Location; -import com.google.devtools.skylark.common.LocationRange; - -/** An issue found by the linter. */ -public class Issue { - public final String category; - public final String message; - public final LocationRange location; - - public Issue(String category, String message, LocationRange location) { - this.category = category; - this.message = message; - this.location = location; - } - - public static Issue create(String category, String message, Location location) { - return new Issue(category, message, LocationRange.from(location)); - } - - @Override - public String toString() { - return location + ": " + message + " [" + category + "]"; - } - - public String prettyPrint(String path) { - return path + ":" + toString(); - } - - public static int compareLocation(Issue i1, Issue i2) { - return LocationRange.compare(i1.location, i2.location); - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java deleted file mode 100644 index 93253419ec26c9..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java +++ /dev/null @@ -1,193 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.events.EventKind; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import java.io.IOException; -import java.nio.charset.StandardCharsets; -import java.nio.file.Files; -import java.nio.file.NoSuchFileException; -import java.nio.file.Path; -import java.util.ArrayList; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; - -/** - * Main class of the linter library. - * - *

Most users of the linter library should only need to use this class. - */ -public class Linter { - private static final String PARSE_ERROR_CATEGORY = "parse-error"; - /** Map of all single-file checks and their names. */ - private static final ImmutableMap nameToCheck = - ImmutableMap.builder() - .put("bad-operation", BadOperationChecker::check) - .put("bad-recursive-glob", NativeRecursiveGlobChecker::check) - .put("control-flow", ControlFlowChecker::check) - .put("deprecated-api", DeprecatedApiChecker::check) - .put("docstring", DocstringChecker::check) - .put("load", LoadStatementChecker::check) - .put("naming", NamingConventionsChecker::check) - .put("no-effect", StatementWithoutEffectChecker::check) - .put("usage", UsageChecker::check) - .build(); - /** Map of all multi-file checks and their names. */ - private static final ImmutableMap nameToMultiFileCheck = - ImmutableMap.builder() - .put("deprecation", DeprecationChecker::check) - .build(); - - /** Function to read files (can be changed for testing). */ - private FileFacade fileFacade = DEFAULT_FILE_FACADE; - - private static final FileFacade DEFAULT_FILE_FACADE = - new FileFacade() { - @Override - public boolean fileExists(Path path) { - return Files.exists(path); - } - - @Override - public byte[] readBytes(Path path) throws IOException { - return Files.readAllBytes(path); - } - }; - - private boolean singleFileMode = false; - private final Set disabledChecks = new LinkedHashSet<>(); - private final Set disabledCategories = new LinkedHashSet<>(); - - public Linter setFileContentsReader(FileFacade reader) { - this.fileFacade = reader; - return this; - } - - public Linter disableCheck(String checkName) { - if (!nameToCheck.containsKey(checkName)) { - throw new IllegalArgumentException("Unknown check '" + checkName + "' cannot be disabled."); - } - disabledChecks.add(checkName); - return this; - } - - public Linter disableCategory(String categoryName) { - disabledCategories.add(categoryName); - return this; - } - - /** Disables checks that require analyzing multiple files. */ - public Linter setSingleFileMode() { - singleFileMode = true; - return this; - } - - /** - * Runs all checks on the given file. - * - * @param path path of the file - * @return list of issues found in that file - */ - public List lint(Path path) throws IOException { - path = path.toAbsolutePath(); - String content = new String(fileFacade.readBytes(path), StandardCharsets.ISO_8859_1); - List issues = new ArrayList<>(); - BuildFileAST ast = - BuildFileAST.parseString( - event -> { - if (event.getKind() == EventKind.ERROR || event.getKind() == EventKind.WARNING) { - issues.add( - Issue.create(PARSE_ERROR_CATEGORY, event.getMessage(), event.getLocation())); - } - }, - content); - for (Map.Entry entry : nameToCheck.entrySet()) { - if (disabledChecks.contains(entry.getKey())) { - continue; - } - issues.addAll(entry.getValue().check(ast)); - } - if (!singleFileMode) { - for (Map.Entry entry : nameToMultiFileCheck.entrySet()) { - if (disabledChecks.contains(entry.getKey())) { - continue; - } - issues.addAll(entry.getValue().check(path, ast, fileFacade)); - } - } - issues.removeIf(issue -> disabledCategories.contains(issue.category)); - issues.sort(Issue::compareLocation); - return issues; - } - - /** - * Interface with a function that reads a file. - * - *

This is useful because we can use a fake for testing. - */ - @FunctionalInterface - public interface FileFacade { - - /** - * Reads a file path to bytes. - * - *

This operation may be repeated for the same file. - */ - byte[] readBytes(Path path) throws IOException; - - /** - * Reads a file and parses it to an AST. - * - *

The default implementation silently ignores syntax errors. - */ - default BuildFileAST readAst(Path path) throws IOException { - String contents = new String(readBytes(path), StandardCharsets.ISO_8859_1); - return BuildFileAST.parseString(event -> {}, contents); - } - - /** - * Checks whether a given file exists. - * - *

The default implementation invokes readBytes and returns false if {@link - * NoSuchFileException} is thrown, true otherwise. - */ - default boolean fileExists(Path path) { - try { - readBytes(path); - } catch (NoSuchFileException e) { - return false; - } catch (IOException e) { - // This method shouldn't throw. - } - return true; - } - } - - /** Allows to invoke a check. */ - @FunctionalInterface - public interface Check { - List check(BuildFileAST ast); - } - - /** Allows to invoke a check. */ - @FunctionalInterface - public interface MultiFileCheck { - List check(Path path, BuildFileAST ast, FileFacade fileFacade); - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/LoadStatementChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/LoadStatementChecker.java deleted file mode 100644 index 2e21be3b36c870..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/LoadStatementChecker.java +++ /dev/null @@ -1,51 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.LoadStatement; -import com.google.devtools.build.lib.syntax.Statement; -import com.google.devtools.skylark.common.DocstringUtils; -import java.util.ArrayList; -import java.util.List; - -/** Checks that load statements are at the top of a file (after the docstring). */ -public class LoadStatementChecker { - private static final String LOAD_AT_TOP_CATEGORY = "load-at-top"; - - private LoadStatementChecker() {} - - public static List check(BuildFileAST ast) { - List issues = new ArrayList<>(); - List statements = ast.getStatements(); - int firstStatementIndex = DocstringUtils.extractDocstring(statements) == null ? 0 : 1; - boolean loadStatementsExpected = true; - for (int i = firstStatementIndex; i < statements.size(); i++) { - Statement statement = statements.get(i); - if (statement instanceof LoadStatement) { - if (!loadStatementsExpected) { - issues.add( - Issue.create( - LOAD_AT_TOP_CATEGORY, - "load statement should be at the top of the file (after the docstring)", - statement.getLocation())); - } - } else { - loadStatementsExpected = false; - } - } - return issues; - } -} 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 deleted file mode 100644 index 3531d21cd24d0d..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/NamingConventionsChecker.java +++ /dev/null @@ -1,201 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import com.google.devtools.build.lib.events.Location; -import com.google.devtools.build.lib.syntax.ASTNode; -import com.google.devtools.build.lib.syntax.AssignmentStatement; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.Expression; -import com.google.devtools.build.lib.syntax.FuncallExpression; -import com.google.devtools.build.lib.syntax.Identifier; -import com.google.devtools.skylark.skylint.Environment.NameInfo; -import com.google.devtools.skylark.skylint.Environment.NameInfo.Kind; -import java.util.ArrayList; -import java.util.List; - -/** - * Checks the adherence to Skylark naming conventions. - * - *

    - *
  • Functions and parameters should be lower_snake_case and all other identifiers should be - * lower_snake_case (for variables) or UPPER_SNAKE_CASE (for constants). - *
  • Providers are required to be UpperCamelCase. A variable FooBar is considered a provider if - * it appears in an assignment of the form "FooBar = provider(...)". - *
  • Shadowing of builtins (e.g. "True = False", "def fail()") is not allowed. - *
  • The single-letter variable names 'O', 'l', 'I' are disallowed since they're easy to - * confuse. - *
  • Multi-underscore names ('__', '___', etc.) are disallowed. - *
  • Single-underscore names may only be written to, as in "a, _ = tuple". They may not be read, - * as in "f(_)". - *
- */ -// TODO(skylark-team): Check that UPPERCASE_VARIABLES are never mutated -public class NamingConventionsChecker extends AstVisitorWithNameResolution { - private static final String NAME_WITH_WRONG_CASE_CATEGORY = "name-with-wrong-case"; - private static final String PROVIDER_NAME_ENDS_IN_INFO_CATEGORY = "provider-name-suffix"; - private static final String CONFUSING_NAME_CATEGORY = "confusing-name"; - private static final ImmutableList CONFUSING_NAMES = ImmutableList.of("O", "I", "l"); - private static final ImmutableSet BUILTIN_NAMES; - - private final List issues = new ArrayList<>(); - - static { - Environment env = Environment.defaultBazel(); - BUILTIN_NAMES = - env.getNameIdsInCurrentBlock() - .stream() - .map(id -> env.getNameInfo(id).name) - .collect(ImmutableSet.toImmutableSet()); - } - - public static List check(BuildFileAST ast) { - NamingConventionsChecker checker = new NamingConventionsChecker(); - checker.visit(ast); - return checker.issues; - } - - @Override - public void visit(AssignmentStatement node) { - // Check for the pattern "FooBar = provider(...)" because CamelCase for provider names is OK - Expression lvalue = node.getLValue().getExpression(); - Expression rhs = node.getExpression(); - if (lvalue instanceof Identifier && rhs instanceof FuncallExpression) { - Expression function = ((FuncallExpression) rhs).getFunction(); - if (function instanceof Identifier && ((Identifier) function).getName().equals("provider")) { - checkProviderName(((Identifier) lvalue).getName(), lvalue.getLocation()); - visit(rhs); - return; - } - } - super.visit(node); - } - - 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 UpperCamelCase and end in \"Info\" (for providers; example: FizzBuzzInfo)", - location)); - } - } - - private void checkLowerSnakeCase(String name, Location location) { - if (!isLowerSnakeCase(name)) { - issues.add( - Issue.create( - NAME_WITH_WRONG_CASE_CATEGORY, - "identifier '" + name + "' should be lower_snake_case", - location)); - } - } - - private void checkProviderName(String name, Location location) { - if (!isUpperCamelCase(name)) { - issues.add( - Issue.create( - NAME_WITH_WRONG_CASE_CATEGORY, - "provider name '" + name + "' should be UpperCamelCase", - location)); - } - if (!name.endsWith("Info")) { - issues.add( - Issue.create( - PROVIDER_NAME_ENDS_IN_INFO_CATEGORY, - "provider name '" + name + "' should end in the suffix 'Info'", - location) - ); - } - } - - private void checkNameNotConfusing(String name, Location location) { - if (CONFUSING_NAMES.contains(name)) { - issues.add( - Issue.create( - CONFUSING_NAME_CATEGORY, - "never use 'l', 'I', or 'O' as names " - + "(they're too easily confused with 'I', 'l', or '0')", - location)); - } - if (BUILTIN_NAMES.contains(name)) { - issues.add( - Issue.create( - CONFUSING_NAME_CATEGORY, - "identifier '" + name + "' shadows a builtin; please pick a different name", - location)); - } - if (name.chars().allMatch(c -> c == '_') && name.length() >= 2) { - issues.add( - Issue.create( - CONFUSING_NAME_CATEGORY, - "identifier '" - + name - + "' consists only of underscores; please pick a different name", - location)); - } - } - - @Override - void use(Identifier identifier) { - if (identifier.getName().equals("_")) { - issues.add( - Issue.create( - CONFUSING_NAME_CATEGORY, - "don't use '_' as an identifier, only to ignore the result in an assignment", - identifier.getLocation())); - } - } - - @Override - void declare(String name, ASTNode node) { - NameInfo nameInfo = env.resolveExistingName(name); - if (nameInfo.kind == Kind.IMPORTED) { - // Users may not have control over imported names, so ignore them: - return; - } - checkNameNotConfusing(name, node.getLocation()); - if (nameInfo.kind == Kind.PARAMETER || nameInfo.kind == Kind.FUNCTION) { - checkLowerSnakeCase(nameInfo.name, node.getLocation()); - } else { - checkIdentifier(nameInfo.name, node.getLocation()); - } - } - - private static boolean isUpperCamelCase(String name) { - if (name.startsWith("_")) { - name = name.substring(1); // private providers are allowed - } - return !name.contains("_") && Character.isUpperCase(name.charAt(0)); - } - - private static boolean isSnakeCase(String name) { - return isUpperSnakeCase(name) || isLowerSnakeCase(name); - } - - private static boolean isUpperSnakeCase(String name) { - return name.equals(name.toUpperCase()); - } - - private static boolean isLowerSnakeCase(String name) { - return name.equals(name.toLowerCase()); - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/NativeRecursiveGlobChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/NativeRecursiveGlobChecker.java deleted file mode 100644 index 9857d6655bb1fb..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/NativeRecursiveGlobChecker.java +++ /dev/null @@ -1,132 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; -import com.google.devtools.build.lib.syntax.Argument; -import com.google.devtools.build.lib.syntax.AssignmentStatement; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.Expression; -import com.google.devtools.build.lib.syntax.FuncallExpression; -import com.google.devtools.build.lib.syntax.Identifier; -import com.google.devtools.build.lib.syntax.ListLiteral; -import com.google.devtools.build.lib.syntax.StringLiteral; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; - -/** - * Checks the adherence to Skylark best practices for recursive globs. - * - *

Recursive globs should be used sparingly and not for files containing source code. This check - * flags incorrect usage of recurisive globs, for languages known to be problematic. - */ -public class NativeRecursiveGlobChecker extends AstVisitorWithNameResolution { - - /* List of instances of glob(**) we found. */ - private final List issues = new ArrayList<>(); - - /* List of known variables we've encountered for finding indirect use of glob(**) */ - private final Map vars = new HashMap<>(); - - /* List of variables that were found in globs, but had not yet been resolved in SkyLark - * processing. Example: - * - * native.glob([my_var]) - * - * ... - * - * my_var = "**\/*.java" - */ - private final Set waitingFor = new HashSet<>(); - - private static final String BAD_RECURSIVE_GLOB = "bad-recursive-glob"; - - public static List check(BuildFileAST ast) { - NativeRecursiveGlobChecker checker = new NativeRecursiveGlobChecker(); - checker.visit(ast); - return checker.issues; - } - - private void evaluateInclude(Expression exp) { - if (exp.kind() == Expression.Kind.STRING_LITERAL) { - StringLiteral str = (StringLiteral) exp; - String value = str.getValue(); - if (value.contains("**") && value.endsWith("*.java")) { - issues.add( - Issue.create( - BAD_RECURSIVE_GLOB, - "go/build-style#globs " - + "Do not use recursive globs for Java source files. glob() on multiple " - + "directories is error prone and can cause serious maintenance problems for " - + "BUILD files.", exp.getLocation())); - } - } else if (exp.kind() == Expression.Kind.IDENTIFIER) { - Identifier id = (Identifier) exp; - if (vars.containsKey(id)) { - evaluateInclude(vars.get(id)); - } else { - waitingFor.add(id); - } - } - } - - @Override - public void visit(FuncallExpression node) { - if (node.getFunction().toString().equals("glob")) { - Argument.Passed include = null; - int index = 0; - List args = node.getArguments(); - for (Argument.Passed a : args) { - if (index == 0 && a.isPositional()) { - include = a; - break; - } else if (index > 1 - && a.isKeyword() - && (a.getName() != null && a.getName().equals("include"))) { - include = a; - break; - } - index++; - } - if (include != null && include.getValue().kind() == Expression.Kind.LIST_LITERAL) { - ListLiteral list = (ListLiteral) include.getValue(); - for (Expression exp : list.getElements()) { - evaluateInclude(exp); - } - } - } - super.visit(node); - } - - @Override - public void visit(AssignmentStatement node) { - super.visit(node); - ImmutableSet lvalues = node.getLValue().boundIdentifiers(); - if (lvalues.size() != 1) { - return; - } - Identifier ident = Iterables.getOnlyElement(lvalues); - vars.put(ident, node.getExpression()); - - if (waitingFor.contains(ident)) { - evaluateInclude(node.getExpression()); - } - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Skylint.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Skylint.java deleted file mode 100644 index 4109ad38d2f377..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Skylint.java +++ /dev/null @@ -1,85 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import java.io.IOException; -import java.nio.file.NoSuchFileException; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.util.ArrayList; -import java.util.List; - -/** The main class for the skylint binary. */ -public class Skylint { - public static void main(String[] args) { - Linter linter = new Linter(); - List paths = new ArrayList<>(); - for (String arg : args) { - if (arg.equals("--single-file")) { - linter.setSingleFileMode(); - } else if (arg.startsWith("--disable-categories=")) { - for (String categoryName : parseArgumentList(arg, "--disable-categories=")) { - linter.disableCategory(categoryName); - } - } else if (arg.startsWith("--disable-checks=")) { - for (String checkName : parseArgumentList(arg, "--disable-checks=")) { - linter.disableCheck(checkName); - } - } else { - paths.add(Paths.get(arg)); - } - } - boolean issuesFound = false; - for (Path path : paths) { - List issues; - try { - issues = linter.lint(path); - } catch (IOException e) { - issuesFound = true; - if (e instanceof NoSuchFileException) { - System.err.println("File not found: " + path); - } else { - System.err.println("Error trying to read " + path); - e.printStackTrace(); - } - continue; - } - if (!issues.isEmpty()) { - issuesFound = true; - for (Issue issue : issues) { - System.out.println(issue.prettyPrint(path.toString())); - } - } - } - System.exit(issuesFound ? 1 : 0); - } - - /** Removes the prefix from the argument and returns the list of comma-separated items. */ - private static List parseArgumentList(String arg, String prefix) { - if (!arg.startsWith(prefix)) { - throw new IllegalArgumentException("Argument doesn't start with prefix " + prefix); - } - List list = new ArrayList<>(); - String[] items = arg.substring(prefix.length()).split(","); - for (String item : items) { - item = item.trim(); - if (item.isEmpty()) { - continue; - } - list.add(item); - } - return list; - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/StatementWithoutEffectChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/StatementWithoutEffectChecker.java deleted file mode 100644 index 4b64a4089d7dab..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/StatementWithoutEffectChecker.java +++ /dev/null @@ -1,96 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.Expression; -import com.google.devtools.build.lib.syntax.ExpressionStatement; -import com.google.devtools.build.lib.syntax.FuncallExpression; -import com.google.devtools.build.lib.syntax.FunctionDefStatement; -import com.google.devtools.build.lib.syntax.ListComprehension; -import com.google.devtools.build.lib.syntax.Statement; -import com.google.devtools.build.lib.syntax.SyntaxTreeVisitor; -import com.google.devtools.skylark.common.DocstringUtils; -import java.util.ArrayList; -import java.util.List; - -/** Checks for statements that have no effect. */ -public class StatementWithoutEffectChecker extends SyntaxTreeVisitor { - private static final String NO_EFFECT_CATEGORY = "no-effect"; - - private final List issues = new ArrayList<>(); - private boolean hasEffect = false; - private boolean topLevel = true; - - public static List check(BuildFileAST ast) { - StatementWithoutEffectChecker checker = new StatementWithoutEffectChecker(); - checker.visit(ast); - return checker.issues; - } - - @Override - public void visit(BuildFileAST ast) { - checkStatementsExceptDocstrings(ast.getStatements(), /*allowVariableDocstrings=*/ true); - } - - @Override - public void visit(FunctionDefStatement node) { - topLevel = false; - checkStatementsExceptDocstrings(node.getStatements(), /*allowVariableDocstrings=*/ false); - topLevel = true; - } - - private void checkStatementsExceptDocstrings( - List stmts, boolean allowVariableDocstrings) { - Statement prev = null; - for (Statement cur : stmts) { - boolean isStringLiteral = DocstringUtils.getStringLiteral(cur) != null; - boolean isVariableDocstring = - allowVariableDocstrings - && isStringLiteral - && DocstringUtils.getAssignedVariableName(prev) != null; - boolean isDocstringAtTop = isStringLiteral && prev == null; - if (!isVariableDocstring && !isDocstringAtTop) { - visit(cur); - } - prev = cur; - } - } - - @Override - public void visit(ExpressionStatement node) { - hasEffect = false; - super.visit(node); - Expression expr = node.getExpression(); - if (expr instanceof FuncallExpression) { - return; // function calls can have an effect - } - if (expr instanceof ListComprehension && topLevel && hasEffect) { - // allow list comprehensions at the top level if they have an effect, e.g. [print(x) for x in - // list] - return; - } - String message = "expression result not used"; - if (expr instanceof ListComprehension && !topLevel) { - message += ". Use a for-loop instead of a list comprehension."; - } - issues.add(Issue.create(NO_EFFECT_CATEGORY, message, node.getLocation())); - } - - @Override - public void visit(FuncallExpression node) { - hasEffect = true; - } -} diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java deleted file mode 100644 index d38932e2e382ec..00000000000000 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java +++ /dev/null @@ -1,328 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.base.Equivalence; -import com.google.common.base.Equivalence.Wrapper; -import com.google.common.collect.LinkedHashMultimap; -import com.google.common.collect.SetMultimap; -import com.google.devtools.build.lib.syntax.ASTNode; -import com.google.devtools.build.lib.syntax.AssignmentStatement; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.Expression; -import com.google.devtools.build.lib.syntax.ExpressionStatement; -import com.google.devtools.build.lib.syntax.FlowStatement; -import com.google.devtools.build.lib.syntax.ForStatement; -import com.google.devtools.build.lib.syntax.FunctionDefStatement; -import com.google.devtools.build.lib.syntax.Identifier; -import com.google.devtools.build.lib.syntax.IfStatement; -import com.google.devtools.build.lib.syntax.IfStatement.ConditionalStatements; -import com.google.devtools.build.lib.syntax.ReturnStatement; -import com.google.devtools.skylark.skylint.Environment.NameInfo; -import com.google.devtools.skylark.skylint.Environment.NameInfo.Kind; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Set; - -/** Checks that every import, private function or variable definition is used somewhere. */ -public class UsageChecker extends AstVisitorWithNameResolution { - private static final String UNUSED_BINDING_CATEGORY = "unused-binding"; - private static final String UNINITIALIZED_VARIABLE_CATEGORY = "uninitialized-variable"; - - private final List issues = new ArrayList<>(); - private UsageInfo ui = UsageInfo.empty(); - private final SetMultimap> idToAllDefinitions = - LinkedHashMultimap.create(); - private final Set> initializationsWithNone = new LinkedHashSet<>(); - - public static List check(BuildFileAST ast) { - UsageChecker checker = new UsageChecker(); - checker.visit(ast); - return checker.issues; - } - - @Override - public void visit(FunctionDefStatement node) { - UsageInfo saved = ui.copy(); - super.visit(node); - ui = UsageInfo.join(Arrays.asList(saved, ui)); - } - - @Override - public void visit(IfStatement node) { - UsageInfo input = ui; - List outputs = new ArrayList<>(); - for (ConditionalStatements clause : node.getThenBlocks()) { - ui = input.copy(); - visit(clause); - outputs.add(ui); - } - ui = input.copy(); - visitBlock(node.getElseBlock()); - outputs.add(ui); - ui = UsageInfo.join(outputs); - } - - @Override - public void visit(ForStatement node) { - visit(node.getCollection()); - visit(node.getVariable()); - UsageInfo noIteration = ui.copy(); - visitBlock(node.getBlock()); - UsageInfo oneIteration = ui.copy(); - // We need to visit the block again in case a variable was reassigned in the last iteration and - // the new value isn't used until the next iteration - visit(node.getVariable()); - visitBlock(node.getBlock()); - UsageInfo manyIterations = ui.copy(); - ui = UsageInfo.join(Arrays.asList(noIteration, oneIteration, manyIterations)); - } - - @Override - public void visit(FlowStatement node) { - ui.reachable = false; - } - - @Override - public void visit(ReturnStatement node) { - super.visit(node); - ui.reachable = false; - } - - @Override - public void visit(ExpressionStatement node) { - super.visit(node); - if (ControlFlowChecker.isFail(node.getExpression())) { - ui.reachable = false; - } - } - - @Override - public void visit(AssignmentStatement node) { - super.visit(node); - /* If a variable is initialized with None, and there exist other assignments to the variable, - * then this initialization is itself considered as a usage. This is because it's good practice - * to place a "declaration" of a variable in a location that dominates all its uses, especially - * so if you want to document the variable. Example: - * - * var = None # don't warn about the unused binding - * if condition: - * var = 0 - * else: - * var = 1 - * - * Unfortunately, as a side-effect, the following won't trigger a warning either: - * - * var = None # doesn't warn either but ideally should - * var = 0 - */ - Expression lhs = node.getLValue().getExpression(); - Expression rhs = node.getExpression(); - if (lhs instanceof Identifier - && rhs instanceof Identifier - && ((Identifier) rhs).getName().equals("None")) { - NameInfo info = env.resolveName(((Identifier) lhs).getName()); - // if it's an initialization: - if (info != null && idToAllDefinitions.get(info.id).size() == 1) { - initializationsWithNone.add(wrapNode(lhs)); - } - } - } - - private void defineIdentifier(NameInfo name, ASTNode node) { - ui.idToLastDefinitions.removeAll(name.id); - ui.idToLastDefinitions.put(name.id, wrapNode(node)); - ui.initializedIdentifiers.add(name.id); - idToAllDefinitions.put(name.id, wrapNode(node)); - } - - @Override - protected void use(Identifier identifier) { - NameInfo info = env.resolveName(identifier.getName()); - // TODO(skylark-team): Don't ignore unresolved symbols in the future but report an error - if (info != null) { - ui.usedDefinitions.addAll(ui.idToLastDefinitions.get(info.id)); - checkInitialized(info, identifier); - } - } - - @Override - protected void declare(String name, ASTNode node) { - NameInfo info = env.resolveExistingName(name); - defineIdentifier(info, node); - } - - @Override - protected void reassign(Identifier ident) { - declare(ident.getName(), ident); - } - - @Override - public void exitBlock() { - Collection ids = env.getNameIdsInCurrentBlock(); - for (Integer id : ids) { - checkUsed(id); - } - super.exitBlock(); - } - - private void checkUsed(Integer id) { - Set> unusedDefinitions = new LinkedHashSet<>(idToAllDefinitions.get(id)); - unusedDefinitions.removeAll(ui.usedDefinitions); - NameInfo nameInfo = env.getNameInfo(id); - String name = nameInfo.name; - if ("_".equals(name) || nameInfo.kind == Kind.BUILTIN) { - return; - } - if ((nameInfo.kind == Kind.LOCAL || nameInfo.kind == Kind.PARAMETER) - && (name.startsWith("_") || name.startsWith("unused_") || name.startsWith("UNUSED_"))) { - // local variables starting with an underscore need not be used - return; - } - if ((nameInfo.kind == Kind.GLOBAL || nameInfo.kind == Kind.FUNCTION) && !name.startsWith("_")) { - // symbol might be loaded in another file - return; - } - String message = "unused binding of '" + name + "'"; - if (nameInfo.kind == Kind.IMPORTED && !nameInfo.name.startsWith("_")) { - message += - ". If you want to re-export a symbol, use the following pattern:\n" - + "\n" - + "load(..., _" - + name - + " = '" - + name - + "', ...)\n" - + name - + " = _" - + name - + "\n" - + "\n" - + "More details in the documentation."; - } else if (nameInfo.kind == Kind.PARAMETER) { - message += - ". If this is intentional, " - + "you can add `_ignore = [, , ...]` to the function body."; - } else if (nameInfo.kind == Kind.LOCAL) { - message += ". If this is intentional, you can use '_' or rename it to '_" + name + "'."; - } - for (Wrapper definition : unusedDefinitions) { - if (initializationsWithNone.contains(definition) && idToAllDefinitions.get(id).size() > 1) { - // initializations with None are OK, cf. visit(AssignmentStatement) above - continue; - } - issues.add( - Issue.create(UNUSED_BINDING_CATEGORY, message, unwrapNode(definition).getLocation())); - } - } - - private void checkInitialized(NameInfo info, Identifier node) { - if (ui.reachable && !ui.initializedIdentifiers.contains(info.id) && info.kind != Kind.BUILTIN) { - issues.add( - Issue.create( - UNINITIALIZED_VARIABLE_CATEGORY, - "variable '" - + info.name - + "' may not have been initialized." - + " If you believe this is wrong, you can add `fail('unreachable')" - + " to the branches where it is not initialized" - + " or initialize it with `None` at the beginning." - + " For more details, have a look at the documentation.", - node.getLocation())); - } - } - - private static class UsageInfo { - /** - * Stores for each variable ID the definitions that are "live", i.e. are the most recent ones on - * some execution path. - * - *

There can be more than one last definition if branches are involved, e.g. if foo: x = 1; - * else x = 2; - */ - private final SetMultimap> idToLastDefinitions; - /** Set of definitions that have already been used at some point. */ - private final Set> usedDefinitions; - /** Set of variable IDs that are initialized. */ - private final Set initializedIdentifiers; - /** - * Whether the current position in the program is reachable. - * - *

This is needed to correctly compute initialized variables. - */ - private boolean reachable; - - private UsageInfo( - SetMultimap> idToLastDefinitions, - Set> usedDefinitions, - Set initializedIdentifiers, - boolean reachable) { - this.idToLastDefinitions = idToLastDefinitions; - this.usedDefinitions = usedDefinitions; - this.initializedIdentifiers = initializedIdentifiers; - this.reachable = reachable; - } - - static UsageInfo empty() { - return new UsageInfo( - LinkedHashMultimap.create(), new LinkedHashSet<>(), new LinkedHashSet<>(), true); - } - - UsageInfo copy() { - return new UsageInfo( - LinkedHashMultimap.create(idToLastDefinitions), - new LinkedHashSet<>(usedDefinitions), - new LinkedHashSet<>(initializedIdentifiers), - reachable); - } - - static UsageInfo join(Collection uis) { - Set initializedInRelevantBranch = new LinkedHashSet<>(); - for (UsageInfo ui : uis) { - if (ui.reachable) { - initializedInRelevantBranch = ui.initializedIdentifiers; - break; - } - } - UsageInfo result = - new UsageInfo( - LinkedHashMultimap.create(), - new LinkedHashSet<>(), - initializedInRelevantBranch, - false); - for (UsageInfo ui : uis) { - result.idToLastDefinitions.putAll(ui.idToLastDefinitions); - result.usedDefinitions.addAll(ui.usedDefinitions); - if (ui.reachable) { - // Only a non-diverging branch can affect the set of initialized variables. - result.initializedIdentifiers.retainAll(ui.initializedIdentifiers); - } - result.reachable |= ui.reachable; - } - return result; - } - } - - private Wrapper wrapNode(ASTNode node) { - return Equivalence.identity().wrap(node); - } - - private ASTNode unwrapNode(Wrapper wrapper) { - return wrapper.get(); - } -} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BUILD b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BUILD deleted file mode 100644 index 717b05c29c47b9..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BUILD +++ /dev/null @@ -1,25 +0,0 @@ -# Tests for the Skylark linter - -java_test( - name = "SkylintTests", - srcs = glob(["*.java"]), - test_class = "com.google.devtools.skylark.skylint.SkylintTests", - deps = [ - "//src/main/java/com/google/devtools/build/lib:events", - "//src/main/java/com/google/devtools/build/lib:packages", - "//src/test/java/com/google/devtools/build/lib:testutil", - "//src/tools/skylark/java/com/google/devtools/skylark/common", - "//src/tools/skylark/java/com/google/devtools/skylark/skylint:skylint_lib", - "//third_party:guava", - "//third_party:junit4", - "//third_party:truth", - ], -) - -filegroup( - name = "srcs", - srcs = glob( - ["**"], - ), - visibility = ["//src:__pkg__"], -) diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java deleted file mode 100644 index 20cd662de72bea..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java +++ /dev/null @@ -1,160 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.truth.Truth; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import java.util.List; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -@RunWith(JUnit4.class) -public class BadOperationCheckerTest { - private static List findIssues(String... lines) { - String content = String.join("\n", lines); - BuildFileAST ast = - BuildFileAST.parseString( - event -> { - throw new IllegalArgumentException(event.getMessage()); - }, - content); - return BadOperationChecker.check(ast); - } - - @Test - public void dictionaryLiteralPlusOperator() { - Truth.assertThat(findIssues("{} + foo").toString()) - .contains( - "1:1-1:8: '+' operator is deprecated and should not be used on dictionaries" - + " [deprecated-plus-dict]"); - Truth.assertThat(findIssues("foo + {}").toString()) - .contains( - "1:1-1:8: '+' operator is deprecated and should not be used on dictionaries" - + " [deprecated-plus-dict]"); - Truth.assertThat(findIssues("foo += {}").toString()) - .contains( - "1:1-1:9: '+=' operator is deprecated and should not be used on dictionaries" - + " [deprecated-plus-dict]"); - } - - @Test - public void dictionaryComprehensionPlusOperator() { - Truth.assertThat(findIssues("{k:v for k,v in []} + foo").toString()) - .contains( - "1:1-1:25: '+' operator is deprecated and should not be used on dictionaries" - + " [deprecated-plus-dict]"); - Truth.assertThat(findIssues("foo + {k:v for k,v in []}").toString()) - .contains( - "1:1-1:25: '+' operator is deprecated and should not be used on dictionaries" - + " [deprecated-plus-dict]"); - Truth.assertThat(findIssues("foo += {k:v for k,v in []}").toString()) - .contains( - "1:1-1:26: '+=' operator is deprecated and should not be used on dictionaries" - + " [deprecated-plus-dict]"); - } - - @Test - public void dictionaryPlusOperatorNested() { - Truth.assertThat(findIssues("foo + ({} + bar)").toString()) - .contains( - "1:7-1:16: '+' operator is deprecated and should not be used on dictionaries" - + " [deprecated-plus-dict]"); - Truth.assertThat(findIssues("foo + (bar + {})").toString()) - .contains( - "1:7-1:16: '+' operator is deprecated and should not be used on dictionaries" - + " [deprecated-plus-dict]"); - } - - @Test - public void depsetPlusOperator() { - Truth.assertThat(findIssues("foo + depset()").toString()) - .contains( - "1:1-1:14: '+' operator is deprecated and should not be used on depsets " - + "[deprecated-plus-depset]"); - - Truth.assertThat(findIssues("foo = depset()", "foo + bar").toString()) - .contains( - "2:1-2:9: '+' operator is deprecated"); - - Truth.assertThat(findIssues("foo = depset()", "bar = foo", "bar + baz").toString()) - .contains( - "3:1-3:9: '+' operator is deprecated"); - - Truth.assertThat(findIssues("foo = depset()", "foo += bar").toString()) - .contains( - "2:1-2:10: '+=' operator is deprecated"); - - Truth.assertThat(findIssues("foo += depset()").toString()) - .contains( - "1:1-1:15: '+=' operator is deprecated"); - } - - @Test - public void dictPlusOperator() { - Truth.assertThat(findIssues("foo + dict()").toString()) - .contains( - "1:1-1:12: '+' operator is deprecated and should not be used on dictionaries " - + "[deprecated-plus-dict]"); - - Truth.assertThat(findIssues("foo = dict()", "foo + bar").toString()) - .contains( - "2:1-2:9: '+' operator is deprecated"); - - Truth.assertThat(findIssues("foo = dict()", "bar = foo", "bar + baz").toString()) - .contains( - "3:1-3:9: '+' operator is deprecated"); - - Truth.assertThat(findIssues("foo = dict()", "foo += bar").toString()) - .contains( - "2:1-2:10: '+=' operator is deprecated"); - - Truth.assertThat(findIssues("foo += dict()").toString()) - .contains( - "1:1-1:13: '+=' operator is deprecated"); - - Truth.assertThat(findIssues("foo += { 5:3 }").toString()) - .contains( - "1:1-1:14: '+=' operator is deprecated"); - - Truth.assertThat(findIssues("foo = { 5:3 }", "bar = foo", "bar + baz").toString()) - .contains( - "3:1-3:9: '+' operator is deprecated"); - } - - @Test - public void pipeOperator() { - Truth.assertThat(findIssues("foo | bar").toString()) - .contains("1:1-1:9: '|' operator is deprecated"); - } - - @Test - public void plusOperatorNoIssue() { - Truth.assertThat(findIssues("foo + bar")).isEmpty(); - Truth.assertThat(findIssues("foo += bar")).isEmpty(); - } - - @Test - public void divisionOperator() { - Truth.assertThat(findIssues("5 / 2").toString()) - .contains("1:1-1:5: '/' operator is deprecated"); - Truth.assertThat(findIssues("5 // 2")).isEmpty(); - } - - @Test - public void augmentedAssignmentOperator() { - Truth.assertThat(findIssues("kwargs['name'] += 'foo'")).isEmpty(); - } -} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java deleted file mode 100644 index 3424be94944a79..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java +++ /dev/null @@ -1,348 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.truth.Truth; -import com.google.devtools.build.lib.events.EventHandler; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import java.util.List; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -@RunWith(JUnit4.class) -public class ControlFlowCheckerTest { - private static List findIssues(EventHandler eventHandler, String... lines) { - String content = String.join("\n", lines); - BuildFileAST ast = BuildFileAST.parseString(eventHandler, content); - return ControlFlowChecker.check(ast); - } - - private static List findIssues(String... lines) { - return findIssues( - event -> { - throw new IllegalArgumentException(event.getMessage()); - }, - lines); - } - - @Test - public void testAnalyzerToleratesTopLevelFail() throws Exception { - Truth.assertThat( - findIssues("fail(\"fail is considered a return, but not at the top level\")")) - .isEmpty(); - } - - @Test - public void testIfElseReturnMissing() throws Exception { - Truth.assertThat( - findIssues( - "def some_function(x):", - " if x:", - " print('foo')", - " else:", - " return x") - .toString()) - .contains( - "1:1-5:12: some but not all execution paths of 'some_function' return a value." - + " If it is intentional, make it explicit using 'return None'." - + " If you know these cannot happen," - + " add the statement `fail('unreachable')` to them." - + " For more details, have a look at the documentation. [missing-return-value]"); - } - - @Test - public void testNestedFunction() { - Truth.assertThat( - findIssues(event -> {}, "def foo():", " def bar():", " pass", " return") - .toString()) - .contains( - "2:3-3:8: bar is a nested function which is not allowed." - + " Consider inlining it or moving it to top-level." - + " For more details, have a look at the Skylark documentation." - + " [nested-function]"); - } - - @Test - public void testIfElseReturnValueMissing() throws Exception { - String messages = - findIssues( - "def some_function(x):", - " if x:", - " return x", - " else:", - " return # missing value") - .toString(); - Truth.assertThat(messages) - .contains( - "1:1-5:10: some but not all execution paths of 'some_function' return a value." - + " If it is intentional, make it explicit using 'return None'." - + " If you know these cannot happen," - + " add the statement `fail('unreachable')` to them." - + " For more details, have a look at the documentation. [missing-return-value]"); - Truth.assertThat(messages) - .contains( - "5:5-5:10: return value missing (you can `return None` if this is desired)" - + " [missing-return-value]"); - } - - @Test - public void testIfElifElseReturnMissing() throws Exception { - Truth.assertThat( - findIssues( - "def f(x):", - " if x:", - " return x", - " elif not x:", - " pass", - " else:", - " return not x") - .toString()) - .contains( - "1:1-7:16: some but not all execution paths of 'f' return a value." - + " If it is intentional, make it explicit using 'return None'." - + " If you know these cannot happen," - + " add the statement `fail('unreachable')` to them." - + " For more details, have a look at the documentation. [missing-return-value]"); - } - - @Test - public void testNestedIfElseReturnMissing() throws Exception { - Truth.assertThat( - findIssues( - "def f(x, y):", - " if x:", - " if y:", - " return y", - " else:", - " print('foo')", - " else:", - " return x") - .toString()) - .contains( - "1:1-8:12: some but not all execution paths of 'f' return a value." - + " If it is intentional, make it explicit using 'return None'." - + " If you know these cannot happen," - + " add the statement `fail('unreachable')` to them." - + " For more details, have a look at the documentation. [missing-return-value]"); - } - - @Test - public void testElseBranchMissing() throws Exception { - Truth.assertThat( - findIssues( - "def some_function(x):", - " if x:", - " return x", - " elif not x:", - " return not x") - .toString()) - .containsMatch( - "1:1-5:16: some but not all execution paths of 'some_function' return a value." - + " .+ \\[missing-return-value\\]"); - } - - @Test - public void testIfAndFallOffEnd() throws Exception { - Truth.assertThat( - findIssues( - "def some_function(x):", - " if x:", - " return x", - " print('foo')", - " # return missing here") - .toString()) - .containsMatch( - "1:1-4:14: some but not all execution paths of 'some_function' return a value." - + " .+ \\[missing-return-value\\]"); - } - - @Test - public void testForAndFallOffEnd() throws Exception { - Truth.assertThat( - findIssues( - "def some_function():", - " for x in []:", - " return x", - " print('foo')", - " # return missing here") - .toString()) - .containsMatch( - "1:1-4:14: some but not all execution paths of 'some_function' return a value." - + " .+ \\[missing-return-value\\]"); - } - - @Test - public void testAlwaysReturnButSometimesWithoutValue() throws Exception { - String messages = - findIssues( - "def some_function(x):", - " if x:", - " return # returns without value here", - " return x") - .toString(); - Truth.assertThat(messages) - .containsMatch( - "1:1-4:10: some but not all execution paths of 'some_function' return a value." - + " .+ \\[missing-return-value\\]"); - Truth.assertThat(messages) - .contains( - "3:5-3:10: return value missing (you can `return None` if this is desired)" - + " [missing-return-value]"); - } - - @Test - public void testUnreachableAfterIf() throws Exception { - String messages = - findIssues( - "def some_function(x):", - " if x:", - " return", - " else:", - " fail('fail')", - " print('This line is unreachable')") - .toString(); - Truth.assertThat(messages).contains("6:3-6:35: unreachable statement [unreachable-statement]"); - } - - @Test - public void testNoUnreachableDuplicates() throws Exception { - List messages = - findIssues( - "def some_function():", - " return", - " print('unreachable1')", - " print('unreachable2')"); - Truth.assertThat(messages).hasSize(1); - } - - @Test - public void testUnreachableAfterBreakContinue() throws Exception { - String messages = - findIssues( - "def some_function(x):", - " for y in x:", - " if y:", - " break", - " else:", - " continue", - " print('unreachable')") - .toString(); - Truth.assertThat(messages).contains("7:5-7:24: unreachable statement [unreachable-statement]"); - } - - @Test - public void testReachableStatements() throws Exception { - Truth.assertThat( - findIssues( - "def some_function(x):", - " if x:", - " return", - " for y in []:", - " if y:", - " continue", - " else:", - " fail('fail')", - " return")) - .isEmpty(); - } - - @Test - public void testIfBeforeReturn() throws Exception { - Truth.assertThat( - findIssues( - "def f(x, y):", - " if x:", - " return x", - " elif not y:", - " print('foo')", - " print('bar')", - " return y")) - .isEmpty(); - } - - @Test - public void testReturnInAllBranches() throws Exception { - Truth.assertThat( - findIssues( - "def f(x, y):", - " if x:", - " return x", - " elif not y:", - " return None", - " else:", - " return y")) - .isEmpty(); - } - - @Test - public void testReturnInNestedIf() throws Exception { - Truth.assertThat( - findIssues( - "def f(x,y):", - " if x:", - " if y:", - " return y", - " else:", - " return not y", - " else:", - " return not x")) - .isEmpty(); - } - - @Test - public void testIfStatementSequence() throws Exception { - Truth.assertThat( - findIssues( - "def f(x,y):", - " if x:", - " print('foo')", - " else:", - " return x", - " print('bar')", - " if y:", - " return x", - " else:", - " return y")) - .isEmpty(); - List issues = - findIssues( - "def f(x,y):", - " if x:", - " return x", - " else:", - " return x", - " # from now on everything's unreachable", - " print('bar')", - " if y:", - " return x", - " # no else branch but doesn't matter since it's unreachable"); - Truth.assertThat(issues).hasSize(1); - Truth.assertThat(issues.toString()) - .contains("7:3-7:14: unreachable statement [unreachable-statement]"); - } - - @Test - public void testCallToFail() throws Exception { - Truth.assertThat( - findIssues( - "def some_function_name(x):", - " if x:", - " fail('bar')", - " else:", - " return x")) - .isEmpty(); - } -} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DependencyAnalyzerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DependencyAnalyzerTest.java deleted file mode 100644 index e34d86ed0829ae..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DependencyAnalyzerTest.java +++ /dev/null @@ -1,237 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.collect.ImmutableMap; -import com.google.common.truth.Truth; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.build.lib.syntax.FunctionDefStatement; -import com.google.devtools.build.lib.syntax.LoadStatement; -import com.google.devtools.build.lib.syntax.Statement; -import com.google.devtools.skylark.skylint.DependencyAnalyzer.DependencyCollector; -import com.google.devtools.skylark.skylint.Linter.FileFacade; -import java.nio.charset.StandardCharsets; -import java.nio.file.NoSuchFileException; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.List; -import java.util.Map; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests for {@link com.google.devtools.skylark.skylint.DependencyAnalyzer} */ -@RunWith(JUnit4.class) -public class DependencyAnalyzerTest { - private static final DependencyCollector> functionCollector = - new DependencyCollector>() { - @Override - public List initInfo(Path path) { - return new ArrayList<>(); - } - - @Override - public List loadDependency( - List currentFileInfo, - LoadStatement stmt, - Path loadedPath, - List loadedFileInfo) { - for (String name : loadedFileInfo) { - currentFileInfo.add(loadedPath + " - " + name); - } - return currentFileInfo; - } - - @Override - public List collectInfo(Path path, BuildFileAST ast, List info) { - for (Statement stmt : ast.getStatements()) { - if (stmt instanceof FunctionDefStatement) { - info.add(((FunctionDefStatement) stmt).getIdentifier().getName()); - } - } - return info; - } - }; - - public static FileFacade toFileFacade(Map files) { - return path -> { - String contents = files.get(path.toString()); - if (contents == null) { - throw new NoSuchFileException(path.toString()); - } - return contents.getBytes(StandardCharsets.ISO_8859_1); - }; - } - - private static List getFunctionNames(Map files, String path) { - return new DependencyAnalyzer<>(toFileFacade(files), functionCollector) - .collectTransitiveInfo(Paths.get(path)); - } - - @Test - public void externalRepositoryDependency() throws Exception { - Map files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/BUILD", "") - .put("/test.bzl", "load('@some-repo//foo:bar.bzl', 'baz')") - .build(); - Truth.assertThat(getFunctionNames(files, "/test.bzl")).isEmpty(); - } - - @Test - public void nonexistentDependency() throws Exception { - Map files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/BUILD", "") - .put("/test.bzl", "load(':does_not_exist.bzl', 'baz')") - .build(); - Truth.assertThat(getFunctionNames(files, "/test.bzl")).isEmpty(); - } - - @Test - public void samePackageDependency() throws Exception { - Map files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/BUILD", "") - .put("/test.bzl", "load(':bar.bzl', 'baz')\nload('//:bar.bzl', 'baz')") - .put("/bar.bzl", "def baz(): pass") - .build(); - Truth.assertThat(getFunctionNames(files, "/test.bzl")) - .isEqualTo(Arrays.asList("/bar.bzl - baz", "/bar.bzl - baz")); - } - - @Test - public void samePackageDependencyWithoutBuildFile() throws Exception { - Map files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/test.bzl", "load(':bar.bzl', 'baz')") - .put("/bar.bzl", "def baz(): pass") - .build(); - Truth.assertThat(getFunctionNames(files, "/test.bzl")).isEmpty(); - } - - @Test - public void subpackageDependency() throws Exception { - Map files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/BUILD", "") - .put("/test.bzl", "load('subpackage:foo.bzl', 'foo')") - .put("/subpackage/BUILD", "") - .put("/subpackage/foo.bzl", "def foo(): pass") - .build(); - Truth.assertThat(getFunctionNames(files, "/test.bzl")) - .isEqualTo(Collections.singletonList("/subpackage/foo.bzl - foo")); - } - - @Test - public void dependencyInSiblingPackage() throws Exception { - Map files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/pkg1/BUILD", "") - .put("/pkg1/test.bzl", "load('//pkg2:bar.bzl', 'bar')") - .put("/pkg2/BUILD", "") - .put("/pkg2/bar.bzl", "def bar(): pass") - .build(); - Truth.assertThat(getFunctionNames(files, "/pkg1/test.bzl")) - .isEqualTo(Collections.singletonList("/pkg2/bar.bzl - bar")); - } - - @Test - public void dependencyInSiblingPackageWithBuildDotBazelFile() throws Exception { - Map files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/pkg1/BUILD.bazel", "") - .put("/pkg1/test.bzl", "load('//pkg2:bar.bzl', 'bar')") - .put("/pkg2/BUILD.bazel", "") - .put("/pkg2/bar.bzl", "def bar(): pass") - .build(); - Truth.assertThat(getFunctionNames(files, "/pkg1/test.bzl")) - .isEqualTo(Collections.singletonList("/pkg2/bar.bzl - bar")); - } - - @Test - public void dependencyLabelWithoutPrefix() throws Exception { - Map files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/test/test.bzl", "load('bar.bzl', 'baz')") - .put("/bar.bzl", "def baz(): pass") - .build(); - Truth.assertThat(getFunctionNames(files, "/test/test.bzl")) - .isEqualTo(Collections.singletonList("/bar.bzl - baz")); - } - - @Test - public void transitiveDependencies() throws Exception { - Map files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/BUILD", "") - .put("/test.bzl", "load(':foo.bzl', 'foo')") - .put("/foo.bzl", "load(':bar.bzl', foo = 'bar')") - .put("/bar.bzl", "def bar(): pass") - .build(); - Truth.assertThat(getFunctionNames(files, "/test.bzl")) - .isEqualTo(Collections.singletonList("/foo.bzl - /bar.bzl - bar")); - } - - @Test - public void diamondDependencies() throws Exception { - Map files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/BUILD", "") - .put("/test.bzl", "load(':foo.bzl', 'foo')\nload(':bar.bzl', 'bar')") - .put("/foo.bzl", "load(':base.bzl', foo = 'base')") - .put("/bar.bzl", "load(':base.bzl', bar = 'base')") - .put("/base.bzl", "def base(): pass") - .build(); - Truth.assertThat(getFunctionNames(files, "/test.bzl")) - .isEqualTo(Arrays.asList("/foo.bzl - /base.bzl - base", "/bar.bzl - /base.bzl - base")); - } - - @Test - public void cyclicDependenciesAreHandledGracefully() throws Exception { - Map files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/BUILD", "") - .put("/test.bzl", "load(':test.bzl', 'foo')\ndef test(): pass") - .build(); - Truth.assertThat(getFunctionNames(files, "/test.bzl")) - .isEqualTo(Collections.singletonList("test")); - - files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/BUILD", "") - .put("/test.bzl", "load(':foo.bzl', 'baz')\ndef test(): pass") - .put("/foo.bzl", "load(':bar.bzl', 'baz')") - .put("/bar.bzl", "load(':foo.bzl', 'baz')") - .build(); - Truth.assertThat(getFunctionNames(files, "/test.bzl")) - .isEqualTo(Collections.singletonList("test")); - } -} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecatedApiCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecatedApiCheckerTest.java deleted file mode 100644 index 4d34ad3d607c8b..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecatedApiCheckerTest.java +++ /dev/null @@ -1,82 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.truth.Truth; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import java.util.List; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -@RunWith(JUnit4.class) -public class DeprecatedApiCheckerTest { - private static List findIssues(String... lines) { - String content = String.join("\n", lines); - BuildFileAST ast = - BuildFileAST.parseString( - event -> { - throw new IllegalArgumentException(event.getMessage()); - }, - content); - return DeprecatedApiChecker.check(ast); - } - - @Test - public void deprecatedCtxMethods() { - Truth.assertThat(findIssues("ctx.action()").toString()) - .contains("1:1-1:10: ctx.action is deprecated: Use ctx.actions.run"); - Truth.assertThat(findIssues("ctx.empty_action").toString()) - .contains("1:1-1:16: ctx.empty_action is deprecated"); - Truth.assertThat(findIssues("ctx.default_provider()").toString()) - .contains("1:1-1:20: ctx.default_provider is deprecated"); - Truth.assertThat(findIssues("PACKAGE_NAME").toString()) - .contains("1:1-1:12: PACKAGE_NAME is deprecated"); - Truth.assertThat(findIssues("f = ctx.outputs.executable").toString()) - .contains("1:5-1:26: ctx.outputs.executable is deprecated"); - Truth.assertThat(findIssues("css_filetype = FileType(['.css'])").toString()) - .contains("1:16-1:23: FileType is deprecated"); - Truth.assertThat(findIssues("native.package()").toString()) - .contains("Call package() in the BUILD file instead"); - - Truth.assertThat(findIssues("ctx.actions()")).isEmpty(); - } - - @Test - public void testRuleImplReturnValue() { - Truth.assertThat( - findIssues("def _impl(ctx): return struct()", "x = rule(implementation=_impl)") - .toString()) - .contains("1:17-1:31: Avoid using the legacy provider syntax."); - - Truth.assertThat( - findIssues( - "def _impl(ctx):", - " if True: return struct()", - " return", - "x = rule(_impl, attrs = {})") - .toString()) - .contains("2:12-2:26: Avoid using the legacy provider syntax."); - - Truth.assertThat( - findIssues( - "def _impl(): return struct()", - "def _impl2(): return []", - "x = rule(", - " implementation=_impl2,", - ")")) - .isEmpty(); - } -} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java deleted file mode 100644 index fffc5bd48013b8..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java +++ /dev/null @@ -1,226 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.collect.ImmutableMap; -import com.google.common.truth.Truth; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import com.google.devtools.skylark.skylint.Linter.FileFacade; -import java.io.IOException; -import java.nio.charset.StandardCharsets; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.util.List; -import java.util.Map; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests the lint done by {@link DeprecationChecker}. */ -@RunWith(JUnit4.class) -public class DeprecationCheckerTest { - private static List findIssuesIgnoringDependencies(String... lines) { - String content = String.join("\n", lines); - BuildFileAST ast = - BuildFileAST.parseString( - event -> { - throw new IllegalArgumentException(event.getMessage()); - }, - content); - return DeprecationChecker.check( - Paths.get("/fake"), ast, _path -> content.getBytes(StandardCharsets.ISO_8859_1)); - } - - @Test - public void symbolDeprecatedInSameFile() { - String errorMessages = - findIssuesIgnoringDependencies( - "def f():", - " g()", - " h()", - " print(x)", - "def g():", - " '''Foo.", - " ", - " Deprecated:", - " Reason.'''", - "def h():", - " '''Bar.", - " ", - " This function is DEPRECATED for some reason.", - " The deprecation should really be documented in a 'Deprecated:' section", - " but the linter should recognize this kind of deprecation as well'''", - "x = 0", - "'''A deprecated variable.", - "", - "Deprecated:", - " Reason.", - "'''") - .toString(); - Truth.assertThat(errorMessages) - .contains("2:3: usage of 'g' is deprecated: Reason. [deprecated-symbol]"); - Truth.assertThat(errorMessages) - .contains( - "3:3: usage of 'h' is deprecated: This function is DEPRECATED for some reason." - + " [deprecated-symbol]"); - Truth.assertThat(errorMessages) - .contains("4:9: usage of 'x' is deprecated: Reason. [deprecated-symbol]"); - } - - @Test - public void deprecatedFunctionAliasing() { - String errorMessages = - findIssuesIgnoringDependencies( - "def f():", - " h = g", - " h()", - "def g():", - " '''Foo.", - " ", - " Deprecated:", - " reason'''") - .toString(); - Truth.assertThat(errorMessages) - .contains("2:7: usage of 'g' is deprecated: reason [deprecated-symbol]"); - } - - @Test - public void functionNotDeprecated() { - Truth.assertThat( - findIssuesIgnoringDependencies( - "def f():", - " g()", - "def g():", - " '''This is a good function.", - "", - " It is emphatically not deprecated.'''")) - .isEmpty(); - } - - @Test - public void noWarningsInsideDeprecatedFunctions() { - Truth.assertThat( - findIssuesIgnoringDependencies( - "def f():", - " '''A deprecated function calling another deprecated function -> no warning", - "", - " Deprecated:", - " This function is deprecated.", - " '''", - " g()", - "", - "def g():", - " '''Another deprecated function", - "", - " Deprecated:", - " This function is deprecated.'''")) - .isEmpty(); - } - - @Test - public void shadowingWorks() { - Truth.assertThat( - findIssuesIgnoringDependencies( - "def f():", - " bad = good", - " bad()", - "def good(): pass", - "def bad():", - " '''This is a deprecated function.", - "", - " Deprecated:", - " reason'''")) - .isEmpty(); - } - - private static final Map files = - ImmutableMap.builder() - .put("/WORKSPACE", "") - .put("/pkg1/BUILD", "") - .put( - "/pkg1/foo.bzl", - String.join( - "\n", - "load('//pkg2:bar.bzl', 'baz', foo = 'bar') # test aliasing", - "load(':qux.bzl', 'qux') # test package label", - "foo()", - "qux()")) - .put( - "/pkg1/qux.bzl", - String.join( - "\n", - "load(':does_not_exist.bzl', 'foo') # test error handling", - "def qux():", - " '''qux", - "", - " Deprecated:", - " qux is deprecated'''", - "return")) - .put("/pkg2/BUILD", "") - .put( - "/pkg2/bar.bzl", - String.join( - "\n", - "load('//pkg2/pkg3:baz.bzl', bar = 'baz') # test aliasing", - "bar()", - "def baz():", - " '''baz", - "", - " Deprecated:", - " baz is deprecated", - " '''")) - .put("/pkg2/pkg3/BUILD", "") - .put( - "/pkg2/pkg3/baz.bzl", - String.join( - "\n", - "def baz():", - " '''baz", - "", - " Deprecated:", - " baz is deprecated", - " '''")) - .build(); - - private static final FileFacade testFileFacade = DependencyAnalyzerTest.toFileFacade(files); - - private static List findIssuesIncludingDependencies(String pathString) throws IOException { - Path path = Paths.get(pathString); - return DeprecationChecker.check(path, testFileFacade.readAst(path), testFileFacade); - } - - @Test - public void deprecatedFunctionsInDirectDependency() throws Exception { - String errorMessages = findIssuesIncludingDependencies("/pkg1/foo.bzl").toString(); - Truth.assertThat(errorMessages) - .contains( - "4:1-4:3: usage of 'qux' (imported from /pkg1/qux.bzl)" - + " is deprecated: qux is deprecated"); - errorMessages = findIssuesIncludingDependencies("/pkg2/bar.bzl").toString(); - Truth.assertThat(errorMessages) - .contains( - "2:1-2:3: usage of 'bar' (imported from /pkg2/pkg3/baz.bzl, named 'baz' there)" - + " is deprecated: baz is deprecated"); - } - - @Test - public void deprecatedFunctionsInTransitiveDependencies() throws Exception { - String errorMessages = findIssuesIncludingDependencies("/pkg1/foo.bzl").toString(); - Truth.assertThat(errorMessages) - .contains( - "3:1-3:3: usage of 'foo' (imported from /pkg2/pkg3/baz.bzl, named 'baz' there)" - + " is deprecated: baz is deprecated"); - } -} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTest.java deleted file mode 100644 index 31090f6527806b..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTest.java +++ /dev/null @@ -1,290 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.truth.Truth; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import java.util.List; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests the lint done by {@link DocstringChecker}. */ -@RunWith(JUnit4.class) -public class DocstringCheckerTest { - private static List findIssues(String... lines) { - String content = String.join("\n", lines); - BuildFileAST ast = - BuildFileAST.parseString( - event -> { - throw new IllegalArgumentException(event.getMessage()); - }, - content); - return DocstringChecker.check(ast); - } - - @Test - public void reportMissingDocString() throws Exception { - String errorMessage = - findIssues( - "# no module docstring", - "def function():", - " # no function docstring", - " print(1) # make sure the function is long enough", - " print(2)", - " print(3)", - " print(4)", - " print(5)") - .toString(); - Truth.assertThat(errorMessage) - .contains("1:1-2:1: file has no module docstring [missing-module-docstring]"); - Truth.assertThat(errorMessage) - .contains( - "2:1-4:2: function 'function' has no docstring" - + " (if this function is intended to be private," - + " the name should start with an underscore: '_function')" - + " [missing-function-docstring]"); - } - - @Test - public void reportMissingParameterDocumentation() throws Exception { - List errors = - findIssues( - "\"\"\" module docstring \"\"\"", - "def f(param1, *args, **kwargs):", - " \"\"\"summary", - "", - " more description", - " \"\"\"", - " pass"); - Truth.assertThat(errors).hasSize(1); - Truth.assertThat(errors.toString()) - .contains( - "3:3-6:5: incomplete docstring: the function parameters are not documented" - + " (no 'Args:' section found)\n" - + "The parameter documentation should look like this:\n" - + "\n" - + "Args:\n" - + " param1: ...\n" - + " *args: ...\n" - + " **kwargs: ...\n" - + "\n" - + " [inconsistent-docstring]"); - } - - @Test - public void reportUndocumentedParameters() throws Exception { - String errorMessage = - findIssues( - "def function(foo, bar, baz):", - " \"\"\"summary", - "", - " Args:", - " bar: blabla", - " \"\"\"", - " pass") - .toString(); - Truth.assertThat(errorMessage) - .contains( - "2:3-6:5: incomplete docstring: parameter 'foo' not documented" - + " [inconsistent-docstring]"); - Truth.assertThat(errorMessage) - .contains( - "2:3-6:5: incomplete docstring: parameter 'baz' not documented" - + " [inconsistent-docstring]"); - } - - @Test - public void reportArgumentsUse() throws Exception { - String errorMessage = - findIssues( - "def function(foo, bar):", - " \"\"\"summary", - "", - " Arguments:", - " foo: bla", - " bar: blabla", - " \"\"\"", - " pass") - .toString(); - Truth.assertThat(errorMessage) - .contains( - "4:3-4:13: Prefer 'Args:' to 'Arguments:' when documenting function arguments." - + " [args-arguments-docstring]"); - } - - @Test - public void reportObsoleteParameterDocumentation() throws Exception { - String errorMessage = - findIssues( - "def function(bar):", - " \"\"\"summary", - "", - " Args:", - " foo: blabla", - " bar: blabla", - " baz: blabla", - " \"\"\"", - " pass") - .toString(); - Truth.assertThat(errorMessage) - .contains( - "2:3-8:5: inconsistent docstring: parameter 'foo' appears in docstring" - + " but not in function signature [inconsistent-docstring]"); - Truth.assertThat(errorMessage) - .contains( - "2:3-8:5: inconsistent docstring: parameter 'baz' appears in docstring" - + " but not in function signature [inconsistent-docstring]"); - } - - @Test - public void reportParametersDocumentedInDifferentOrder() throws Exception { - String errorMessage = - findIssues( - "def function(p1, p2):", - " \"\"\"summary", - "", - " Args:", - " p2: blabla", - " p1: blabla", - " \"\"\"", - " pass") - .toString(); - Truth.assertThat(errorMessage) - .contains( - "2:3-7:5:" - + " inconsistent docstring: order of parameters differs from function signature\n" - + "Declaration order: p1, p2\n" - + "Documentation order: p2, p1\n" - + " [inconsistent-docstring]"); - } - - @Test - public void reportInvalidDocstringFormat() throws Exception { - String errorMessage = findIssues("\"\"\"summary", "missing blank line\"\"\"").toString(); - Truth.assertThat(errorMessage) - .contains( - "2:1-2:18: bad docstring format: " - + "the one-line summary should be followed by a blank line [bad-docstring-format]"); - - errorMessage = - findIssues( - "def f():", - " \"\"\"summary", - "", - " foo", - " bad indentation in this line", - "\"\"\"") - .toString(); - Truth.assertThat(errorMessage) - .contains( - "5:1-5:29: bad docstring format: " - + "line indented too little (here: 1 spaces; expected: 2 spaces)" - + " [bad-docstring-format]"); - } - - @Test - public void reportMissingReturnDocumentation() throws Exception { - List errors = - findIssues( - "\"\"\" module docstring \"\"\"", - "def f():", - " \"\"\"summary", - "", - " more description", - " \"\"\"", - " return True"); - Truth.assertThat(errors).hasSize(1); - Truth.assertThat(errors.toString()) - .contains( - "3:3-6:5: incomplete docstring: the return value is not documented" - + " (no 'Returns:' section found) [inconsistent-docstring]"); - } - - @Test - public void dontReportReturnDocumentationIfNoReturnValue() throws Exception { - Truth.assertThat( - findIssues( - "\"\"\" module docstring \"\"\"", - "def f():", - " \"\"\"summary", - "", - " more description", - " \"\"\"", - " return")) - .isEmpty(); - } - - @Test - public void dontReportExistingDocstrings() throws Exception { - Truth.assertThat( - findIssues( - "\"\"\"This is a module docstring", - "\n\"\"\"", - "def function():", - " \"\"\" This is a function docstring", - "", - " \"\"\"")) - .isEmpty(); - } - - @Test - public void dontReportSingleLineDocstring() throws Exception { - Truth.assertThat( - findIssues( - "\"\"\"module docstring\"\"\"", - "def function(param1, param2):", - " \"\"\"single line docstring is fine\"\"\"", - " return True")) - .isEmpty(); - } - - @Test - public void dontReportPrivateFunctionWithoutDocstring() throws Exception { - Truth.assertThat( - findIssues( - "\"\"\" Module docstring\n\"\"\"", - "def _private_function():", - " pass # no docstring necessary for private functions")) - .isEmpty(); - } - - @Test - public void dontReportShortFunctionWithoutDocstring() throws Exception { - Truth.assertThat(findIssues("\"\"\"Module docstring\"\"\"", "def function():", " pass")) - .isEmpty(); - } - - @Test - public void dontReportCorrectFunctionDocstring() throws Exception { - Truth.assertThat( - findIssues( - "\"\"\" module docstring \"\"\"", - "def function(param1, param2, *args, **kwargs):", - " \"\"\"summary", - "", - " Args:", - " param1: foo", - " param2 (foo, bar): baz", - " *args: foo", - " **kwargs: bar", - "", - " Returns:", - " True", - " \"\"\"", - " return True")) - .isEmpty(); - } -} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringUtilsTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringUtilsTest.java deleted file mode 100644 index 897421f43902fd..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringUtilsTest.java +++ /dev/null @@ -1,520 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.truth.Truth; -import com.google.devtools.skylark.common.DocstringUtils; -import com.google.devtools.skylark.common.DocstringUtils.DocstringInfo; -import com.google.devtools.skylark.common.DocstringUtils.DocstringParseError; -import com.google.devtools.skylark.common.DocstringUtils.ParameterDoc; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests the {@link DocstringUtils} class. */ -@RunWith(JUnit4.class) -public class DocstringUtilsTest { - @Test - public void oneLineDocstring() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = DocstringUtils.parseDocstring("summary", 0, errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getParameters()).isEmpty(); - Truth.assertThat(info.getLongDescription()).isEmpty(); - Truth.assertThat(errors).isEmpty(); - } - - @Test - public void missingBlankLineAfterSummary() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = DocstringUtils.parseDocstring("summary\nfoo", 0, errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getParameters()).isEmpty(); - Truth.assertThat(info.getLongDescription()).isEqualTo("foo"); - Truth.assertThat(errors.toString()) - .contains("2: the one-line summary should be followed by a blank line"); - } - - @Test - public void multiParagraphDocstring() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = DocstringUtils.parseDocstring("summary\n\nfoo\n\nbar\n", 0, errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getParameters()).isEmpty(); - Truth.assertThat(info.getLongDescription()).isEqualTo("foo\n\nbar"); - Truth.assertThat(errors).isEmpty(); - } - - @Test - public void inconsistentIndentation() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring("summary\n" + "\n" + " foo\n" + "bar\n ", 2, errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getParameters()).isEmpty(); - Truth.assertThat(info.getLongDescription()).isEqualTo("foo\nbar"); - Truth.assertThat(errors.toString()) - .contains("4: line indented too little (here: 0 spaces; expected: 2 spaces)"); - - errors = new ArrayList<>(); - info = - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " baseline indentation\n" - + " more indentation can be useful (e.g. for example code)\n" - + " ", - 2, - errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getParameters()).isEmpty(); - Truth.assertThat(info.getLongDescription()) - .isEqualTo( - "baseline indentation\n more indentation can be useful (e.g. for example code)"); - Truth.assertThat(errors).isEmpty(); - } - - @Test - public void inconsistentIndentationInSection() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + "Returns:\n" - + " only one space indentation\n" - + "unindented line after", - 0, - errors); - Truth.assertThat(info.getReturns()).isEqualTo("only one space indentation"); - Truth.assertThat(info.getLongDescription()).isEqualTo("unindented line after"); - Truth.assertThat(errors.toString()) - .contains( - "4: text in a section has to be indented by two spaces" - + " (relative to the left margin of the docstring)"); - Truth.assertThat(errors.toString()).contains("5: end of section without blank line"); - } - - @Test - public void inconsistentIndentationInParameters() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + "Args:\n" - + " param0: two spaces indentation\n" - + " param1: only one space indentation\n" - + " only two spaces indentation in continued line\n" - + "unindented line after", - 0, - errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getParameters()).hasSize(2); - ParameterDoc param0 = info.getParameters().get(0); - Truth.assertThat(param0.getDescription()).isEqualTo("two spaces indentation"); - ParameterDoc param1 = info.getParameters().get(1); - Truth.assertThat(param1.getDescription()) - .isEqualTo("only one space indentation\n only two spaces indentation in continued line"); - Truth.assertThat(errors.toString()) - .contains("5: inconsistent indentation of parameter lines (before: 2; here: 1 spaces)"); - Truth.assertThat(errors.toString()).contains("7: end of 'Args' section without blank line"); - } - - @Test - public void whitespaceOnlyLinesCountAsBlank() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring( - "summary\n" - + " \n" // if not blank, would have too much indent - + " description\n" - + " \n" // if not blank, would have too little indent - + " Args:\n" - + " foo: foo description\n" - + " \n" // if not blank, would be indented just the right amount to end param - // description but not Args section - + " Returns:\n" - + " returns description\n" - + " \n" // if not blank, would be section content that's indented too little - + " ", - 4, - errors); - Truth.assertThat(info.getParameters()).hasSize(1); - Truth.assertThat(info.getParameters().get(0).getDescription()).isEqualTo("foo description"); - Truth.assertThat(info.getReturns()).isEqualTo("returns description"); - Truth.assertThat(errors).isEmpty(); - } - - @Test - public void closingQuoteMustBeProperlyIndented() throws Exception { - List errors = new ArrayList<>(); - DocstringUtils.parseDocstring("summary", 4, errors); - Truth.assertThat(errors).isEmpty(); - - errors = new ArrayList<>(); - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " more description", - 4, errors); - Truth.assertThat(errors.toString()) - .contains("3: closing docstring quote should be on its own line, indented the same as the " - + "opening quote"); - - errors = new ArrayList<>(); - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " more description\n" - + " ", // too little - 4, errors); - Truth.assertThat(errors.toString()) - .contains("4: closing docstring quote should be indented the same as the opening quote"); - - errors = new ArrayList<>(); - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " more description\n" - + " ", // too much - 4, errors); - Truth.assertThat(errors.toString()) - .contains("4: closing docstring quote should be indented the same as the opening quote"); - - errors = new ArrayList<>(); - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " more description\n" - + "", // too little (empty line special case) - 4, errors); - Truth.assertThat(errors.toString()) - .contains("4: closing docstring quote should be indented the same as the opening quote"); - } - - @Test - public void emptySection() throws Exception { - List errors = new ArrayList<>(); - DocstringUtils.parseDocstring( - "summary\n" + "\n" + "Args:\n" + "More description.\n", 0, errors); - Truth.assertThat(errors.toString()).contains("3: section is empty or badly formatted"); - - errors = new ArrayList<>(); - DocstringUtils.parseDocstring( - "summary\n" + "\n" + "Returns:\n" + "More description\n", 0, errors); - Truth.assertThat(errors.toString()).contains("3: section is empty"); - - errors = new ArrayList<>(); - DocstringUtils.parseDocstring( - "summary\n" + "\n" + "Deprecated:\n" + "More description\n", 0, errors); - Truth.assertThat(errors.toString()).contains("3: section is empty"); - } - - @Test - public void emptyParamDescription() throws Exception { - List errors = new ArrayList<>(); - DocstringUtils.parseDocstring("summary\n" + "\n" + "Args:\n" + "" + " foo: \n\n", 0, errors); - Truth.assertThat(errors.toString()).contains("4: empty parameter description for 'foo'"); - } - - @Test - public void sectionOnOneLine() throws Exception { - List errors = new ArrayList<>(); - DocstringUtils.parseDocstring("summary\n" + "\n" + "Returns: foo\n", 0, errors); - Truth.assertThat(errors).hasSize(1); - Truth.assertThat(errors.get(0).toString()) - .startsWith("3: the return value should be documented in a section"); - } - - @Test - public void docstringReturn() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " Returns:\n" - + " line 1\n" - + "\n" - + " line 2\n" - + " ", - 2, - errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getReturns()).isEqualTo("line 1\n\nline 2"); - Truth.assertThat(info.getLongDescription()).isEmpty(); - Truth.assertThat(errors).isEmpty(); - } - - @Test - public void docstringDeprecated() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " Deprecated:\n" - + " line 1\n" - + "\n" - + " line 2\n" - + " ", - 2, - errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getDeprecated()).isEqualTo("line 1\n\nline 2"); - Truth.assertThat(info.getLongDescription()).isEmpty(); - Truth.assertThat(errors).isEmpty(); - } - - @Test - public void docstringDeprecatedTheWrongWay() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring("summary\n" + "\n" + " Deprecated: foo\n ", 2, errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getDeprecated()).isEqualTo("Deprecated: foo"); - Truth.assertThat(info.getLongDescription()).isEqualTo("Deprecated: foo"); - Truth.assertThat(errors).hasSize(1); - Truth.assertThat(errors.get(0).toString()) - .startsWith( - "3: use a 'Deprecated:' section for deprecations, similar to a 'Returns:' section"); - - errors = new ArrayList<>(); - info = DocstringUtils.parseDocstring( - "summary\n" + "\n" + " This is DEPRECATED.\n ", - 2, - errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getDeprecated()).isEqualTo("This is DEPRECATED."); - Truth.assertThat(info.getLongDescription()).isEqualTo("This is DEPRECATED."); - Truth.assertThat(errors).hasSize(1); - Truth.assertThat(errors.get(0).toString()) - .startsWith( - "3: use a 'Deprecated:' section for deprecations, similar to a 'Returns:' section"); - } - - @Test - public void docstringParameters() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " Args:\n" - + " param1: multi-\n" - + " line\n" - + " param2 (mutable, unused): bar\n" - + " *args: args\n" - + " **kwargs: kwargs\n" - + " ", - 2, - errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getParameters()).hasSize(4); - Truth.assertThat(info.getLongDescription()).isEmpty(); - ParameterDoc firstParam = info.getParameters().get(0); - ParameterDoc secondParam = info.getParameters().get(1); - ParameterDoc thirdParam = info.getParameters().get(2); - ParameterDoc fourthParam = info.getParameters().get(3); - - Truth.assertThat(firstParam.getParameterName()).isEqualTo("param1"); - Truth.assertThat(firstParam.getAttributes()).isEmpty(); - Truth.assertThat(firstParam.getDescription()).isEqualTo("multi-\n line"); - - Truth.assertThat(secondParam.getParameterName()).isEqualTo("param2"); - Truth.assertThat(secondParam.getAttributes()).isEqualTo(Arrays.asList("mutable", "unused")); - Truth.assertThat(secondParam.getDescription()).isEqualTo("bar"); - - Truth.assertThat(thirdParam.getParameterName()).isEqualTo("*args"); - Truth.assertThat(thirdParam.getAttributes()).isEmpty(); - Truth.assertThat(thirdParam.getDescription()).isEqualTo("args"); - - Truth.assertThat(fourthParam.getParameterName()).isEqualTo("**kwargs"); - Truth.assertThat(fourthParam.getAttributes()).isEmpty(); - Truth.assertThat(fourthParam.getDescription()).isEqualTo("kwargs"); - - Truth.assertThat(errors).isEmpty(); - } - - @Test - public void docstringParametersWithBlankLines() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " Args:\n" - + " param1: multi-\n" - + "\n" - + " line\n" - + "\n" - + " param2: foo\n" - + " ", - 2, - errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getParameters()).hasSize(2); - Truth.assertThat(info.getLongDescription()).isEmpty(); - ParameterDoc firstParam = info.getParameters().get(0); - ParameterDoc secondParam = info.getParameters().get(1); - - Truth.assertThat(firstParam.getParameterName()).isEqualTo("param1"); - Truth.assertThat(firstParam.getAttributes()).isEmpty(); - Truth.assertThat(firstParam.getDescription()).isEqualTo("multi-\n\n line"); - - Truth.assertThat(secondParam.getParameterName()).isEqualTo("param2"); - Truth.assertThat(secondParam.getAttributes()).isEmpty(); - Truth.assertThat(secondParam.getDescription()).isEqualTo("foo"); - - Truth.assertThat(errors).isEmpty(); - } - - @Test - public void sectionNotPrecededByNewline() throws Exception { - List errors = new ArrayList<>(); - DocstringUtils.parseDocstring("summary\n" + "\n" + " foo\n" + " Args:", 2, errors); - Truth.assertThat(errors.toString()).contains("4: section should be preceded by a blank line"); - errors = new ArrayList<>(); - DocstringUtils.parseDocstring("summary\n" + "\n" + " foo\n" + " Returns:", 2, errors); - Truth.assertThat(errors.toString()).contains("4: section should be preceded by a blank line"); - errors = new ArrayList<>(); - DocstringUtils.parseDocstring("summary\n" + "\n" + " foo\n" + " Deprecated:", 2, errors); - Truth.assertThat(errors.toString()).contains("4: section should be preceded by a blank line"); - } - - @Test - public void duplicatedSectionsError() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " Args:\n" - + " param1: foo\n" - + "\n" - + " Args:\n" - + " param2: bar\n" - + "\n" - + " Returns:\n" - + " foo\n" - + "\n" - + " Returns:\n" - + " bar\n" - + "\n" - + " Deprecated:\n" - + " foo\n" - + "\n" - + " Deprecated:\n" - + " bar\n" - + "\n" - + " description", - 2, - errors); - Truth.assertThat(info.getParameters()).hasSize(2); - Truth.assertThat(errors.toString()).contains("6: duplicate 'Args:' section"); - Truth.assertThat(errors.toString()).contains("12: duplicate 'Returns:' section"); - Truth.assertThat(errors.toString()).contains("18: duplicate 'Deprecated:' section"); - } - - @Test - public void sectionsInWrongOrderError() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " Deprecated:\n" - + " bar\n" - + "\n" - + " Returns:\n" - + " foo\n" - + "\n" - + " Args:\n" - + " param1: foo\n" - + "\n" - + " description\n" - + " ", - 2, - errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getParameters()).hasSize(1); - Truth.assertThat(info.getReturns()).isEqualTo("foo"); - Truth.assertThat(info.getDeprecated()).isEqualTo("bar"); - Truth.assertThat(info.getLongDescription()).isEqualTo("description"); - Truth.assertThat(errors.toString()) - .contains("9: 'Args:' section should go before the 'Returns:' section"); - Truth.assertThat(errors.toString()) - .contains("9: 'Args:' section should go before the 'Deprecated:' section"); - Truth.assertThat(errors.toString()) - .contains("6: 'Returns:' section should go before the 'Deprecated:' section"); - Truth.assertThat(errors.toString()) - .contains("12: description body should go before the special sections"); - } - - @Test - public void noRepeatedErrorAboutWrongOrder() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " Args:\n" - + " param1: foo\n" - + "\n" - + " line 1\n" - + " line 2\n" - + " ", - 2, - errors); - Truth.assertThat(info.getSummary()).isEqualTo("summary"); - Truth.assertThat(info.getParameters()).hasSize(1); - Truth.assertThat(info.getLongDescription()).isEqualTo("line 1\nline 2"); - Truth.assertThat(errors).hasSize(1); - Truth.assertThat(errors.get(0).toString()) - .isEqualTo("6: description body should go before the special sections"); - } - - @Test - public void invalidParameterDoc() throws Exception { - List errors = new ArrayList<>(); - DocstringInfo info = - DocstringUtils.parseDocstring( - "summary\n" - + "\n" - + " Args:\n" - + " invalid parameter doc\n" - + "\n" - + " description", - 2, - errors); - Truth.assertThat(info.getParameters()).isEmpty(); - Truth.assertThat(errors.toString()) - .contains( - "4: invalid parameter documentation" - + " (expected format: \"parameter_name: documentation\")."); - } - - @Test - public void parseErrorContainsCorrectLine() throws Exception { - List errors = new ArrayList<>(); - DocstringUtils.parseDocstring( - "summary\n" + "\n" + " Args:\n" + " invalid parameter doc\n", 2, errors); - Truth.assertThat(errors.get(0).getLine()).isEqualTo(" invalid parameter doc"); - } -} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java deleted file mode 100644 index d496ef71b25f54..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java +++ /dev/null @@ -1,66 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.truth.Truth; -import java.nio.charset.StandardCharsets; -import java.nio.file.NoSuchFileException; -import java.nio.file.Paths; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests for {@link com.google.devtools.skylark.skylint.Linter}. */ -@RunWith(JUnit4.class) -public class LinterTest { - @Test - public void testAllCheckersAreRun() throws Exception { - final String file = - String.join( - "\n", - "def function():", - " '''A function.", - " ", - " Deprecated:", - " Reason'''", - " return", - " 'unreachable and unused string literal'", - "_unusedVar = function() + {}", - "load(':dep.bzl', 'bar')"); - final String errorMessages = - new Linter() - .setFileContentsReader( - p -> { - if ("/foo.bzl".equals(p.toString())) { - return file.getBytes(StandardCharsets.ISO_8859_1); - } else { - throw new NoSuchFileException(p.toString()); - } - }) - .lint(Paths.get("/foo.bzl")) - .toString(); - Truth.assertThat(errorMessages).contains("'+' operator is deprecated"); // bad operation checker - Truth.assertThat(errorMessages).contains("unreachable statement"); // control flow checker - Truth.assertThat(errorMessages).contains("'function' is deprecated"); // deprecation checker - Truth.assertThat(errorMessages).contains("has no module docstring"); // docstring checker - Truth.assertThat(errorMessages) - .contains("load statement should be at the top"); // load statement checker - Truth.assertThat(errorMessages) - .contains("should be lower_snake_case"); // naming conventions checker - Truth.assertThat(errorMessages) - .contains("expression result not used"); // checker statements without effect - Truth.assertThat(errorMessages).contains("unused binding of"); // usage checker - } -} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LoadStatementCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LoadStatementCheckerTest.java deleted file mode 100644 index 35857e3722d45d..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LoadStatementCheckerTest.java +++ /dev/null @@ -1,76 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.truth.Truth; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import java.util.List; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests the lint done by {@link LoadStatementChecker}. */ -@RunWith(JUnit4.class) -public class LoadStatementCheckerTest { - private static List findIssues(String... lines) { - String content = String.join("\n", lines); - BuildFileAST ast = - BuildFileAST.parseString( - event -> { - throw new IllegalArgumentException(event.getMessage()); - }, - content); - return LoadStatementChecker.check(ast); - } - - @Test - public void loadStatementsAtTheTop() { - Truth.assertThat( - findIssues( - "'''Docstring'''", - "load(':foo.bzl', 'foo')", - "load(':bar.bzl', 'bar')", - "foo = 'bar'")) - .isEmpty(); - Truth.assertThat(findIssues("load(':foo.bzl', 'foo')", "foo = 'bar'")).isEmpty(); - Truth.assertThat(findIssues("")).isEmpty(); - Truth.assertThat(findIssues("'''Docstring'''")).isEmpty(); - } - - @Test - public void loadStatementNotAtTheTop() { - String errorMessage = - findIssues( - "'''Docstring'''", - "print('This statement should be after the load.')", - "load(':foo.bzl', 'foo')") - .toString(); - Truth.assertThat(errorMessage) - .contains( - "3:1-3:23: load statement should be at the top of the file (after the docstring)" - + " [load-at-top]"); - errorMessage = - findIssues( - "'''Docstring'''", - "load(':foo.bzl', 'foo')", - "print('foo')", - "load(':bar.bzl', 'bar')") - .toString(); - Truth.assertThat(errorMessage) - .contains( - "4:1-4:23: load statement should be at the top of the file (after the docstring)" - + " [load-at-top]"); - } -} 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 deleted file mode 100644 index cec3a81965d54c..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NamingConventionsCheckerTest.java +++ /dev/null @@ -1,138 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.truth.Truth; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import java.util.List; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests the lint done by {@link com.google.devtools.skylark.skylint.NamingConventionsChecker}. */ -@RunWith(JUnit4.class) -public class NamingConventionsCheckerTest { - private static List findIssues(String... lines) { - String content = String.join("\n", lines); - BuildFileAST ast = - BuildFileAST.parseString( - event -> { - throw new IllegalArgumentException(event.getMessage()); - }, - content); - return NamingConventionsChecker.check(ast); - } - - @Test - 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)" - + " 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) - .contains("'BadParameterName' should be lower_snake_case [name-with-wrong-case]"); - Truth.assertThat(errorMessage) - .contains( - "'badLocalVariableName' 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]"); - } - - @Test - public void testConfusingNames() throws Exception { - String errorMessage = findIssues("O = 0", "def fail():", " True = False").toString(); - Truth.assertThat(errorMessage) - .contains( - "never use 'l', 'I', or 'O' as names" - + " (they're too easily confused with 'I', 'l', or '0') [confusing-name]"); - Truth.assertThat(errorMessage) - .contains( - "identifier 'fail' shadows a builtin; please pick a different name [confusing-name]"); - Truth.assertThat(errorMessage) - .contains( - "identifier 'True' shadows a builtin; please pick a different name [confusing-name]"); - } - - @Test - public void testUnderscoreNames() throws Exception { - Truth.assertThat(findIssues("a, _ = (1, 2) # underscore to ignore assignment")).isEmpty(); - Truth.assertThat(findIssues("_ = 1", "print(_)").toString()) - .contains( - "2:7-2:7:" - + " don't use '_' as an identifier, only to ignore the result in an assignment" - + " [confusing-name]"); - Truth.assertThat(findIssues("__ = 1").toString()) - .contains( - "1:1-1:2:" - + " identifier '__' consists only of underscores; please pick a different name" - + " [confusing-name]"); - } - - @Test - public void testImportsNoIssues() throws Exception { - Truth.assertThat( - findIssues("load(':foo.bzl', 'badName', 'BadName', 'O', 'I', 'l', '_', '__', 'fail')")) - .isEmpty(); - } - - @Test - 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 - public void testProviderNameMustBeCamelCase() throws Exception { - Truth.assertThat(findIssues("FooBarInfo = provider()")).isEmpty(); - Truth.assertThat(findIssues("_FooBarInfo = provider()")).isEmpty(); - Truth.assertThat(findIssues("foo_bar = provider()").toString()) - .contains("provider name 'foo_bar' should be UpperCamelCase [name-with-wrong-case]"); - } - - @Test - public void testProviderNameMustEndInInfo() throws Exception { - Truth.assertThat(findIssues("FooBar = provider()").toString()) - .contains("provider name 'FooBar' should end in the suffix 'Info' [provider-name-suffix]"); - } - - @Test - public void testNoDuplicates() throws Exception { - Truth.assertThat(findIssues("def foo():", " badName = 1", " badName += 1")).hasSize(1); - Truth.assertThat(findIssues("def foo():", " Bad = 1", " [[] for Bad in []]")).hasSize(2); - } -} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NativeRecursiveGlobCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NativeRecursiveGlobCheckerTest.java deleted file mode 100644 index e0eda4f876d3c4..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/NativeRecursiveGlobCheckerTest.java +++ /dev/null @@ -1,91 +0,0 @@ -// Copyright 2018 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.truth.Truth; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import java.util.List; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** - * Tests the lint done by {@link com.google.devtools.skylark.skylint.NativeRecursiveGlobChecker}. - */ -@RunWith(JUnit4.class) -public class NativeRecursiveGlobCheckerTest { - private static List findIssues(String... lines) { - String content = String.join("\n", lines); - BuildFileAST ast = - BuildFileAST.parseString( - event -> { - throw new IllegalArgumentException(event.getMessage()); - }, - content); - return NativeRecursiveGlobChecker.check(ast); - } - - @Test - public void testGoodJavaGlob() throws Exception { - List issues = - findIssues("java_library(", " name = \"foo\",", " srcs = glob([\"*.java\"]),", ")"); - Truth.assertThat(issues).isEmpty(); - } - - @Test - public void testCCGlob() throws Exception { - List issues = - findIssues("cc_library(", " name = \"foo\",", " srcs = glob([\"**/*.cc\"]),", ")"); - // We don't currently test for cc globs. - Truth.assertThat(issues).isEmpty(); - } - - @Test - public void testBadJavaGlob() throws Exception { - String errorMessage = - findIssues("java_library(", " name = \"foo\",", " srcs = glob([\"**/*.java\"]),", ")") - .toString(); - Truth.assertThat(errorMessage) - .contains("go/build-style#globs Do not use recursive globs for Java source files."); - } - - @Test - public void testPredefinedJavaGlobVar() throws Exception { - String errorMessage = - findIssues( - "my_var = \"**/*.java\"", - "java_library(", - " name = \"foo\",", - " srcs = glob([my_var]),", - ")") - .toString(); - Truth.assertThat(errorMessage) - .contains("go/build-style#globs Do not use recursive globs for Java source files."); - } - - @Test - public void testPostdefinedJavaGlobVar() throws Exception { - String errorMessage = - findIssues( - "java_library(", - " name = \"foo\",", - " srcs = glob([my_var]),", - ")", - "my_var = \"**/*.java\"") - .toString(); - Truth.assertThat(errorMessage) - .contains("go/build-style#globs Do not use recursive globs for Java source files."); - } -} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/SkylintTests.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/SkylintTests.java deleted file mode 100644 index 8a1e705d6dfb91..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/SkylintTests.java +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.devtools.build.lib.testutil.ClasspathSuite; -import org.junit.runner.RunWith; - -/** All tests for skylint. */ -@RunWith(ClasspathSuite.class) -public class SkylintTests {} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/StatementWithoutEffectCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/StatementWithoutEffectCheckerTest.java deleted file mode 100644 index 242db3496bf069..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/StatementWithoutEffectCheckerTest.java +++ /dev/null @@ -1,97 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.truth.Truth; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import java.util.List; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -@RunWith(JUnit4.class) -public class StatementWithoutEffectCheckerTest { - private static List findIssues(String... lines) { - String content = String.join("\n", lines); - BuildFileAST ast = - BuildFileAST.parseString( - event -> { - throw new IllegalArgumentException(event.getMessage()); - }, - content); - return StatementWithoutEffectChecker.check(ast); - } - - @Test - public void reportUselessExpressionStatements() throws Exception { - String messages = - findIssues("1", "len", "'string'", "'a'.len", "1 + 1", "[1, 2, 3]").toString(); - Truth.assertThat(messages).contains("1:1-1:1: expression result not used [no-effect]"); - Truth.assertThat(messages).contains("2:1-2:3: expression result not used [no-effect]"); - Truth.assertThat(messages).contains("3:1-3:8: expression result not used [no-effect]"); - Truth.assertThat(messages).contains("4:1-4:7: expression result not used [no-effect]"); - Truth.assertThat(messages).contains("5:1-5:5: expression result not used [no-effect]"); - Truth.assertThat(messages).contains("6:1-6:9: expression result not used [no-effect]"); - } - - @Test - public void testListComprehensions() throws Exception { - Truth.assertThat(findIssues("[x for x in []] # has no effect").toString()) - .contains("1:1-1:15: expression result not used"); - Truth.assertThat( - findIssues( - "[print(x) for x in range(5)] # allowed because top-level and has an effect")) - .isEmpty(); - Truth.assertThat( - findIssues( - "def f():", " [print(x) for x in range(5)] # should be replaced by for-loop") - .toString()) - .contains( - "2:3-2:30: expression result not used." - + " Use a for-loop instead of a list comprehension. [no-effect]"); - } - - @Test - public void testDocstrings() throws Exception { - Truth.assertThat( - findIssues( - "\"\"\" docstring \"\"\"", - "x = 0", - "'''A useless variable.'''", - "def f():", - " \"\"\" docstring \"\"\"")) - .isEmpty(); - Truth.assertThat( - findIssues("def f():", " x = 0", " '''Local variables can't have docstrings.'''") - .toString()) - .contains("3:3-3:46: expression result not used [no-effect]"); - } - - @Test - public void dontReportStatementsWithEffect() throws Exception { - Truth.assertThat( - findIssues( - "print('test')", - "[print(a) for a in range(10)]", - "fail('foo')", - "def f():", - " for a in range(5):", - " if a == 0:", - " print(a)", - " else:", - " fail('foo')")) - .isEmpty(); - } -} diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java deleted file mode 100644 index e95c52c75e5d92..00000000000000 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java +++ /dev/null @@ -1,362 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.skylark.skylint; - -import com.google.common.truth.Truth; -import com.google.devtools.build.lib.syntax.BuildFileAST; -import java.util.List; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests the lint done by {@link NamingConventionsChecker}. */ -@RunWith(JUnit4.class) -public class UsageCheckerTest { - private static List findIssues(String... lines) { - String content = String.join("\n", lines); - BuildFileAST ast = - BuildFileAST.parseString( - event -> { - throw new IllegalArgumentException(event.getMessage()); - }, - content); - return UsageChecker.check(ast); - } - - @Test - public void reportUnusedImports() throws Exception { - String message = findIssues("load(':foo.bzl', 'x', 'y', _z = 'Z')").toString(); - Truth.assertThat(message) - .contains( - "1:18-1:20: unused binding of 'x'. If you want to re-export a symbol," - + " use the following pattern:\n" - + "\n" - + "load(..., _x = 'x', ...)\n" - + "x = _x\n" - + "\n" - + "More details in the documentation." - + " [unused-binding]"); - Truth.assertThat(message) - .contains( - "1:23-1:25: unused binding of 'y'. If you want to re-export a symbol," - + " use the following pattern:\n" - + "\n" - + "load(..., _y = 'y', ...)\n" - + "y = _y\n" - + "\n" - + "More details in the documentation." - + " [unused-binding]"); - Truth.assertThat(message).contains("1:28-1:29: unused binding of '_z' [unused-binding]"); - } - - @Test - public void reportUnusedGlobals() throws Exception { - String message = findIssues("_UNUSED = len([])", "def _unused(): pass").toString(); - Truth.assertThat(message).contains("1:1-1:7: unused binding of '_UNUSED' [unused-binding]"); - Truth.assertThat(message).contains("2:5-2:11: unused binding of '_unused' [unused-binding]"); - } - - @Test - public void reportUnusedLocals() throws Exception { - String message = findIssues("def some_function(param):", " local, local2 = 1, 3").toString(); - Truth.assertThat(message).contains("1:19-1:23: unused binding of 'param'"); - Truth.assertThat(message) - .contains( - "you can add `_ignore = [, , ...]` to the function body." - + " [unused-binding]"); - Truth.assertThat(message).contains("2:3-2:7: unused binding of 'local'"); - Truth.assertThat(message) - .contains("you can use '_' or rename it to '_local'. [unused-binding]"); - Truth.assertThat(message).contains("2:10-2:15: unused binding of 'local2'"); - Truth.assertThat(message) - .contains("you can use '_' or rename it to '_local2'. [unused-binding]"); - } - - @Test - public void reportUnusedComprehensionVariable() throws Exception { - String message = findIssues("[[2 for y in []] for x in []]").toString(); - Truth.assertThat(message).contains("1:9-1:9: unused binding of 'y'"); - Truth.assertThat(message).contains("you can use '_' or rename it to '_y'. [unused-binding]"); - Truth.assertThat(message).contains("1:22-1:22: unused binding of 'x'"); - Truth.assertThat(message).contains("you can use '_' or rename it to '_x'. [unused-binding]"); - } - - @Test - public void reportShadowingVariable() throws Exception { - Truth.assertThat( - findIssues( - "def some_function_name_foo_bar_baz_qux():", - " x = [[] for x in []]", - " print(x)") - .toString()) - .contains("2:15-2:15: unused binding of 'x'"); - } - - @Test - public void reportShadowedVariable() throws Exception { - Truth.assertThat(findIssues("def some_function():", " x = [x for x in []]").toString()) - .contains("2:3-2:3: unused binding of 'x'"); - } - - @Test - public void reportReassignedUnusedVariable() throws Exception { - Truth.assertThat(findIssues("def some_function():", " x = 1", " x += 2").toString()) - .contains("3:3-3:3: unused binding of 'x'"); - } - - @Test - public void reportUnusedBeforeIfElse() throws Exception { - Truth.assertThat( - findIssues( - "def f(y):", - " x = 2", - " if y:", - " x = 3", - " else:", - " x = 4", - " print(x)") - .toString()) - .contains("2:3-2:3: unused binding of 'x'"); - } - - @Test - public void reportUnusedInForLoop() throws Exception { - String messages = - findIssues( - "def some_function_foo_bar_baz_qux():", - " for x in []:", - " print(x)", - " x = 2") - .toString(); - Truth.assertThat(messages).contains("4:5-4:5: unused binding of 'x'"); - } - - @Test - public void reportUninitializedAfterIfElifElse() throws Exception { - String message = - findIssues( - "def some_function(a, b):", - " if a:", - " y = 2", - " elif b:", - " pass", - " else:", - " y = 1", - " y += 2", - " print(y)") - .toString(); - Truth.assertThat(message) - .containsMatch( - "8:3-8:3: variable 'y' may not have been initialized. .+ \\[uninitialized-variable\\]"); - } - - @Test - public void reportUninitializedAfterForLoop() throws Exception { - String message = - findIssues("def some_function():", " for _ in []:", " y = 1", " print(y)").toString(); - Truth.assertThat(message) - .containsMatch( - "4:9-4:9: variable 'y' may not have been initialized. .+ \\[uninitialized-variable\\]"); - } - - @Test - public void dontReportAlwaysInitializedInNestedIf() throws Exception { - Truth.assertThat( - findIssues( - "def some_function(a, b):", - " if a:", - " if b:", - " x = b", - " else:", - " x = a", - " else:", - " x = not a", - " return x")) - .isEmpty(); - } - - @Test - public void dontReportAlwaysInitializedBecauseUnreachable() throws Exception { - Truth.assertThat( - findIssues( - "def some_function(a, b):", - " if a:", - " y = 1", - " elif b:", - " return", - " else:", - " fail('fail')", - " print(y)", - " for _ in []:", - " if a:", - " break", - " elif b:", - " continue", - " else:", - " z = 2", - " print(z)")) - .isEmpty(); - } - - @Test - public void dontReportUsedAsParameterDefault() throws Exception { - Truth.assertThat( - findIssues( - "_x = 1", - "def foo(y=_x):", - " print(y)", - "", - "foo()" - ) - ).isEmpty(); - } - - @Test - public void dontReportUsedAfterIf() throws Exception { - Truth.assertThat( - findIssues( - "def some_function(parameter):", - " x = 2", - " if parameter:", - " x = 3", - " print(x)")) - .isEmpty(); - } - - @Test - public void dontReportUsedInNextIteration() throws Exception { - Truth.assertThat( - findIssues( - "def some_function_foo_bar_baz_qux():", - " x = 0", - " for _ in []:", - " print(x)", - " x += 1", - " return x", - "", - "def foo():", - " x = \"xyz\"", - " for i in range(5):", - " if i % 2 == 1:", - " print(x)", - " else:", - " x = \"abc\"")) - .isEmpty(); - } - - @Test - public void dontReportUnusedBuiltins() throws Exception { - Truth.assertThat(findIssues()).isEmpty(); - } - - @Test - public void dontReportPublicGlobals() throws Exception { - Truth.assertThat( - findIssues( - "GLOBAL = 0", - "def global_function_name_foo_bar_baz_qux(parameter):", - " print(parameter)")) - .isEmpty(); - } - - @Test - public void dontReportUsedGlobals() throws Exception { - Truth.assertThat( - findIssues( - "_GLOBAL = 0", - "def public_function():", - " _global_function(_GLOBAL)", - "def _global_function(param):", - " print(param)")) - .isEmpty(); - } - - @Test - public void dontReportUsedLocals() throws Exception { - Truth.assertThat( - findIssues( - "def f(x,y):", - " a = x", - " b = x", - " if x:", - " a = y", - " if y:", - " b = a", - " print(b)")) - .isEmpty(); - } - - @Test - public void dontReportUnderscore() throws Exception { - Truth.assertThat(findIssues("GLOBAL = [1 for _ in []]")).isEmpty(); - } - - @Test - public void dontReportLocalsStartingWithUnderscore() throws Exception { - Truth.assertThat(findIssues("def f(_param):", " _local = [[] for _x in []]")).isEmpty(); - Truth.assertThat(findIssues("def f(unused_param):", " unused_local = [[] for unused_x in []]")) - .isEmpty(); - Truth.assertThat(findIssues("def f():", " UNUSED_CONSTANT = 'unused'")) - .isEmpty(); - } - - @Test - public void dontReportInitializationWithNoneAsDeclaration() throws Exception { - Truth.assertThat( - findIssues( - "def foo(bar):", - " baz = None # here should be no unused warning", - " # because we want to allow people to 'declare' a variable in one location", - " if bar:", - " baz = 0", - " else:", - " baz = 1", - " print(baz)")) - .isEmpty(); - } - - @Test - public void reportUnusedInitializationWithNone() throws Exception { - Truth.assertThat( - findIssues("def foo():", " baz = None # warn here because 'baz' is never used") - .toString()) - .contains("2:3-2:5: unused binding of 'baz'"); - } - - @Test - public void reportSubsequentInitializations() throws Exception { - Truth.assertThat( - findIssues( - "def foo():", - " baz = None", - " baz = None # do warn here (not an initialization)") - .toString()) - .contains("3:3-3:5: unused binding of 'baz'"); - Truth.assertThat( - findIssues( - "def foo():", - " baz = None", - " baz = 0 # do warn here (it's a regular assignment)") - .toString()) - .contains("3:3-3:5: unused binding of 'baz'"); - Truth.assertThat( - findIssues( - "def foo():", - " baz = 0", - " baz = None # do warn here (not an initialization)") - .toString()) - .contains("3:3-3:5: unused binding of 'baz'"); - } -}