Skip to content

Commit

Permalink
Misleading indent var warn (#25207)
Browse files Browse the repository at this point in the history
This was caused by the incorrect location reporting for variables. If
the code above is 4-indented, and then location of the name is used,
that name is also 4-indented, code flags as misleading.

As an example:
```Chapel
      for 1..10 do
          writeln("Hello, world!");
      var unrelated = "hi";
```

Note that `unrelated` and `writeln` happen to be aligned, which is
sufficient for the false positive.

Reported by @alvaradoo -- thanks!

The solution is to steal the logic from IncorrectIndentation that avoids
AST nodes that report their locations oddly.

Reviewed by @jabraham17 -- thanks!

## Testing
- [x] `start_test test/chplcheck`.
  • Loading branch information
DanilaFe authored Jun 11, 2024
2 parents 00d7194 + 6182459 commit c84a1de
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 12 deletions.
5 changes: 5 additions & 0 deletions test/chplcheck/MisleadingIndentation.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,9 @@ writeln("second thing");
writeln(i);
writeln("second thing");

proc f() {
for 1..10 do
writeln("Hello, world!");
var unrelated = "hi";
}
}
5 changes: 5 additions & 0 deletions test/chplcheck/MisleadingIndentation.good-fixit
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,9 @@ writeln(i);
writeln(i);
writeln("second thing");

proc f() {
for 1..10 do
writeln("Hello, world!");
var unrelated = "hi";
}
}
42 changes: 30 additions & 12 deletions tools/chplcheck/src/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,31 @@ def variables(node: AstNode):
yield from variables(child)


def might_incorrectly_report_location(node: AstNode) -> bool:
"""
Some Dyno AST nodes do not return locations in the way that we expect.
For instance, some NamedDecl nodes currently use the name as the location,
which does not indicate their actual indentation. Rules that depend on
indentation should leave these variables alone.
"""

# some NamedDecl nodes currently use the name as the location, which
# does not indicate their actual indentation.
#
# https://github.com/chapel-lang/chapel/issues/25208
if isinstance(node, (VarLikeDecl, TupleDecl, ForwardingDecl)):
return True

# private function locations are bugged and don't include the 'private'
# keyword.
#
# https://github.com/chapel-lang/chapel/issues/24818
elif isinstance(node, (Function, Use, Import)) and node.visibility() != "":
return True

return False


def fixit_remove_unused_node(
node: AstNode,
lines: Optional[List[str]] = None,
Expand Down Expand Up @@ -403,6 +428,8 @@ def MisleadingIndentation(context: Context, root: AstNode):
for child in root:
if isinstance(child, Comment):
continue
if might_incorrectly_report_location(child):
continue
yield from MisleadingIndentation(context, child)

if prev is not None:
Expand Down Expand Up @@ -433,6 +460,8 @@ def MisleadingIndentation(context: Context, root: AstNode):
for blockchild in reversed(list(grandchildren[-1])):
if isinstance(blockchild, Comment):
continue
if might_incorrectly_report_location(blockchild):
continue
prev = blockchild
prevloop = child
break
Expand Down Expand Up @@ -640,22 +669,11 @@ def unwrap_intermediate_block(node: AstNode) -> Optional[AstNode]:
if isinstance(child, Comment):
continue

# some NamedDecl nodes currently use the name as the location, which
# does not indicate their actual indentation.
if isinstance(child, (VarLikeDecl, TupleDecl, ForwardingDecl)):
if might_incorrectly_report_location(child):
continue
# Empty statements get their own warnings, no need to warn here.
elif isinstance(child, EmptyStmt):
continue
# private function locations are bugged and don't include the 'private'
# keyword.
#
# https://github.com/chapel-lang/chapel/issues/24818
elif (
isinstance(child, (Function, Use, Import))
and child.visibility() != ""
):
continue

line, depth = child.location().start()

Expand Down

0 comments on commit c84a1de

Please sign in to comment.