Skip to content

Commit

Permalink
Fix several CLS bugs (#25843)
Browse files Browse the repository at this point in the history
This PR fixes two CLS and adds a workaround for an existing chplcheck
issue.

- Prior to this PR, name locations where not being recorded for enum
element
  ```chapel
  enum A {
    a = 123 // the location is `a = 123`, the name location is `a`
  }
  ``` 
- Prior to this PR, inherit expressions and expressions inside enum
elements were not being properly scope resolved by chapel-py
  ```chapel
  class CC { }
  class C: CC { } // now goto-def works on `CC`

  proc foo param do return 1;
  enum A {
    a = foo // now goto-def works on `foo`
  }
  ```
- Prior to this PR, `chplcheck` would report IncorrectIndentation for
`if`/`else if` chains erroneously. While [that
bug](#25256) still exists,
this PR adds a skip for `else if` (similar to the `public`/`private`
workaround)

Tested that `start_test test/chplcheck` and `make test-cls` both work.

[Reviewed by @DanilaFe]
  • Loading branch information
jabraham17 authored Aug 29, 2024
2 parents da3943d + eca8d83 commit 39f7d38
Show file tree
Hide file tree
Showing 8 changed files with 865 additions and 688 deletions.
1,376 changes: 689 additions & 687 deletions frontend/lib/parsing/bison-chpl-lib.cpp

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions frontend/lib/parsing/chpl.ypp
Original file line number Diff line number Diff line change
Expand Up @@ -2290,6 +2290,7 @@ enum_item:
auto decl = EnumElement::build(BUILDER, LOC(@$),
context->buildAttributeGroup(@$),
$1);
BUILDER->noteDeclNameLocation(decl.get(), LOC(@1));
$$ = STMT(@$, decl.release());
}
| ident_def TASSIGN expr
Expand All @@ -2298,6 +2299,7 @@ enum_item:
context->buildAttributeGroup(@$),
$1,
toOwned($3));
BUILDER->noteDeclNameLocation(decl.get(), LOC(@1));
$$ = STMT(@$, decl.release());
context->clearCommentsBefore(@3);
}
Expand Down
25 changes: 25 additions & 0 deletions test/chplcheck/IncorrectIndentation.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -358,4 +358,29 @@ module IncorrectIndentation {
writeln("hi");
}
}


if 1 < 2 {
writeln("hi");
if 2 < 3 {
writeln("hi");
writeln("??");
}
}
// since else statements aren't reported correctly only the misaligned child statements should warn
if 1 < 2 {
writeln("hi");
writeln("??");
} else if 2 < 3 {
writeln("hi");
writeln("??");
} else {
writeln("hi");
writeln("??");
}
if 1 < 2 {
if 3 < 4 {

}
}
}
5 changes: 5 additions & 0 deletions test/chplcheck/IncorrectIndentation.good
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,9 @@ IncorrectIndentation.chpl:339: node violates rule IncorrectIndentation
IncorrectIndentation.chpl:349: node violates rule IncorrectIndentation
IncorrectIndentation.chpl:353: node violates rule IncorrectIndentation
IncorrectIndentation.chpl:358: node violates rule IncorrectIndentation
IncorrectIndentation.chpl:363: node violates rule IncorrectIndentation
IncorrectIndentation.chpl:373: node violates rule IncorrectIndentation
IncorrectIndentation.chpl:376: node violates rule IncorrectIndentation
IncorrectIndentation.chpl:379: node violates rule IncorrectIndentation
IncorrectIndentation.chpl:382: node violates rule IncorrectIndentation
[Success matching fixit for IncorrectIndentation]
25 changes: 25 additions & 0 deletions test/chplcheck/IncorrectIndentation.good-fixit
Original file line number Diff line number Diff line change
Expand Up @@ -385,4 +385,29 @@ module IncorrectIndentation {
writeln("hi");
}
}


if 1 < 2 {
writeln("hi");
if 2 < 3 {
writeln("hi");
writeln("??");
}
}
// since else statements aren't reported correctly only the misaligned child statements should warn
if 1 < 2 {
writeln("hi");
writeln("??");
} else if 2 < 3 {
writeln("hi");
writeln("??");
} else {
writeln("hi");
writeln("??");
}
if 1 < 2 {
if 3 < 4 {

}
}
}
28 changes: 28 additions & 0 deletions tools/chapel-py/src/resolution.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,30 @@ static const resolution::ResolvedExpression* scopeResolveViaModule(Context* cont
return nullptr;
}

static const resolution::ResolvedExpression*
scopeResolveViaAggregate(Context* context,
const AstNode* aggNode,
const AstNode* node) {
if (auto agg = aggNode->toAggregateDecl()) {
auto& byId = resolution::scopeResolveAggregate(context, agg->id());
if (auto res = byId.byAstOrNull(node)) {
return res;
}
}
return nullptr;
}

static const resolution::ResolvedExpression*
scopeResolveViaEnum(Context* context, const AstNode* enumNode, const AstNode* node) {
if (auto enumDecl = enumNode->toEnum()) {
auto& byId = resolution::scopeResolveEnum(context, enumDecl->id());
if (auto res = byId.byAstOrNull(node)) {
return res;
}
}
return nullptr;
}

// For scope resolution, we handle AST nodes that don't necessarily get
// their own ResolvedExpression (specificlaly, uses/imports), so return an ID
// instead of a ResolvedExpression.
Expand All @@ -130,6 +154,10 @@ static const ID scopeResolveToIdForNode(Context* context, const AstNode* node) {
while (search) {
if (auto rr = scopeResolveViaFunction(context, search, node)) {
return rr->toId();
} else if (auto rr = scopeResolveViaAggregate(context, search, node)) {
return rr->toId();
} else if (auto rr = scopeResolveViaEnum(context, search, node)) {
return rr->toId();
} else if (auto rr = scopeResolveViaModule(context, search, node)) {
return rr->toId();
} else if(auto id = scopeResolveViaVisibilityStmt(context, search, node)) {
Expand Down
70 changes: 70 additions & 0 deletions tools/chpl-language-server/test/basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -320,3 +320,73 @@ async def test_list_references_standard(client: LanguageClient):

assert references is not None
assert len(references) > 10


@pytest.mark.asyncio
async def test_go_to_def_inherit(client: LanguageClient):
"""
Test cases for go-to-definition on inherit expression.
"""

file = """
module foo {
interface II { }
record R: II { }
class CC { }
class C: CC, II { }
module M {
class CC {}
}
class C2: M.CC { }
}
"""

async with source_file(client, file) as doc:
# Definitions link to themselves
await check_goto_decl_def(client, doc, pos((0, 7)), pos((0, 7)))
await check_goto_decl_def(client, doc, pos((1, 10)), pos((1, 10)))
await check_goto_decl_def(client, doc, pos((2, 7)), pos((2, 7)))
await check_goto_decl_def(client, doc, pos((4, 6)), pos((4, 6)))
await check_goto_decl_def(client, doc, pos((5, 6)), pos((5, 6)))
await check_goto_decl_def(client, doc, pos((7, 7)), pos((7, 7)))
await check_goto_decl_def(client, doc, pos((8, 8)), pos((8, 8)))
await check_goto_decl_def(client, doc, pos((10, 6)), pos((10, 6)))

# check the inherit expressions
await check_goto_decl_def(client, doc, pos((2, 10)), pos((1, 10)))
await check_goto_decl_def(client, doc, pos((5, 9)), pos((4, 6)))
await check_goto_decl_def(client, doc, pos((5, 13)), pos((1, 10)))
await check_goto_decl_def(client, doc, pos((10, 10)), pos((7, 7)))
await check_goto_decl_def(client, doc, pos((10, 12)), pos((8, 8)))


@pytest.mark.asyncio
async def test_go_to_enum(client: LanguageClient):
"""
Test cases for go-to-definition on enums.
"""

file = """
proc bar param do return 1;
enum A {
a = bar,
b = 1,
}
var e = A.a;
"""

async with source_file(client, file) as doc:
# Definitions link to themselves
await check_goto_decl_def(client, doc, pos((0, 5)), pos((0, 5)))
await check_goto_decl_def(client, doc, pos((1, 5)), pos((1, 5)))
await check_goto_decl_def(client, doc, pos((2, 2)), pos((2, 2)))
await check_goto_decl_def(client, doc, pos((3, 2)), pos((3, 2)))
await check_goto_decl_def(client, doc, pos((5, 4)), pos((5, 4)))

# check bar
await check_goto_decl_def(client, doc, pos((2, 6)), pos((0, 5)))
# check A.a
await check_goto_decl_def(client, doc, pos((5, 8)), pos((1, 5)))
await check_goto_decl_def(client, doc, pos((5, 10)), pos((2, 2)))
22 changes: 21 additions & 1 deletion tools/chplcheck/src/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,20 @@ def might_incorrectly_report_location(node: AstNode) -> bool:
elif isinstance(node, (Function, Use, Import)) and node.visibility() != "":
return True

# 'else if' statements do not have proper locations
#
# https://github.com/chapel-lang/chapel/issues/25256
elif isinstance(node, Conditional):
parent = node.parent()
grandparent = parent.parent() if parent else None
if (
isinstance(parent, Block)
and parent.block_style() == "implicit"
and grandparent
and isinstance(grandparent, Conditional)
):
return True

return False


Expand Down Expand Up @@ -813,7 +827,13 @@ def unwrap_intermediate_block(node: AstNode) -> Optional[AstNode]:
# var x: int;
# }
elif parent_depth and depth == parent_depth:
yield AdvancedRuleResult(child, anchor=parent_for_indentation)
# conditionals do not support attributes
anchor = (
parent_for_indentation
if not isinstance(parent_for_indentation, Conditional)
else None
)
yield AdvancedRuleResult(child, anchor=anchor)

prev_depth = depth
prev = child
Expand Down

0 comments on commit 39f7d38

Please sign in to comment.