Skip to content

Commit

Permalink
Discourage extremely deeply nested ASTs
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 633595709
  • Loading branch information
cushon authored and Error Prone Team committed May 14, 2024
1 parent 1631f1c commit 1d23d98
Show file tree
Hide file tree
Showing 4 changed files with 174 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* Copyright 2024 The Error Prone Authors.
*
* 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.errorprone.bugpatterns;

import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.Description.NO_MATCH;

import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.sun.source.tree.CompilationUnitTree;
import com.sun.source.tree.Tree;
import javax.inject.Inject;

/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@BugPattern(
summary = "Very deeply nested code may lead to StackOverflowErrors during compilation",
severity = WARNING)
public class DeeplyNested extends BugChecker implements CompilationUnitTreeMatcher {

private final int maxDepth;

@Inject
DeeplyNested(ErrorProneFlags flags) {
maxDepth = flags.getInteger("DeeplyNested:MaxDepth").orElse(1000);
}

@Override
public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
Tree result =
new SuppressibleTreePathScanner<Tree, Integer>(state) {

@Override
public Tree scan(Tree tree, Integer depth) {
if (depth > maxDepth) {
return tree;
}
return super.scan(tree, depth + 1);
}

@Override
public Tree reduce(Tree r1, Tree r2) {
return r1 != null ? r1 : r2;
}
}.scan(state.getPath(), 0);
if (result != null) {
return describeMatch(result);
}
return NO_MATCH;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
import com.google.errorprone.bugpatterns.DeadException;
import com.google.errorprone.bugpatterns.DeadThread;
import com.google.errorprone.bugpatterns.DeduplicateConstants;
import com.google.errorprone.bugpatterns.DeeplyNested;
import com.google.errorprone.bugpatterns.DefaultCharset;
import com.google.errorprone.bugpatterns.DefaultPackage;
import com.google.errorprone.bugpatterns.DepAnn;
Expand Down Expand Up @@ -882,6 +883,7 @@ public static ScannerSupplier warningChecks() {
ComplexBooleanConstant.class,
DateChecker.class,
DateFormatConstant.class,
DeeplyNested.class,
DefaultCharset.class,
DefaultPackage.class,
DeprecatedVariable.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Copyright 2024 The Error Prone Authors.
*
* 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.errorprone.bugpatterns;

import com.google.errorprone.CompilationTestHelper;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public class DeeplyNestedTest {

private final CompilationTestHelper testHelper =
CompilationTestHelper.newInstance(DeeplyNested.class, getClass());

@Test
public void positive() {
testHelper
.addSourceLines(
"Test.java",
"import com.google.common.collect.ImmutableList;",
"class Test {",
" ImmutableList<Integer> xs = ",
" ImmutableList.<Integer>builder()",
" .add(1)",
" .add(2)",
" .add(3)",
" .add(4)",
" .add(5)",
" .add(6)",
" // BUG: Diagnostic contains:",
" .add(7)",
" .add(8)",
" .add(9)",
" .add(10)",
" .build();",
"}")
.setArgs("-XepOpt:DeeplyNested:MaxDepth=10")
.doTest();
}

@Test
public void negative() {
testHelper
.addSourceLines(
"Test.java",
"import com.google.common.collect.ImmutableList;",
"class Test {",
" ImmutableList<Integer> xs = ",
" ImmutableList.<Integer>builder()",
" .add(1)",
" .add(2)",
" .add(3)",
" .build();",
"}")
.setArgs("-XepOpt:DeeplyNested:MaxDepth=100")
.doTest();
}
}
32 changes: 32 additions & 0 deletions docs/bugpattern/DeeplyNested.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
Having an extremely long Java statement with many chained method calls can cause
compilation to fail with a `StackOverflowError` when the compiler tries to
recursively process it.

This is a common problem in generated code.

As an alternative to extremely long chained method calls, e.g. for builders,
consider something like the following for collections with hundreds or thousands
of entries:

```java
private static final ImmutableList<String> FEATURES = createFeatures();

private static final ImmutableList<String> createFeatures() {
ImmutableList.Builder<String> builder = ImmutableList.<String>builder();
builder.add("foo");
builder.add("bar");
...
return builder.build();
}
```

over code like this:

```java
private static final ImmutableList<String> FEATURES =
ImmutableList.<String>builder()
.add("foo")
.add("bar")
...
.build();
```

0 comments on commit 1d23d98

Please sign in to comment.