Skip to content

Commit

Permalink
Ignore order of visibility sections with references inside the type
Browse files Browse the repository at this point in the history
  • Loading branch information
@fourls @Cirras
fourls authored and Cirras committed Nov 5, 2023
1 parent fea49fa commit 56a4d88
Show file tree
Hide file tree
Showing 3 changed files with 265 additions and 10 deletions.
112 changes: 103 additions & 9 deletions delphi-checks/src/main/java/au/com/integradev/delphi/checks/VisibilitySectionOrderCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,31 @@
package au.com.integradev.delphi.checks;

import com.google.common.collect.Maps;
import java.util.ArrayDeque;
import java.util.Collection;
import java.util.Collections;
import java.util.Deque;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.stream.Collectors;
import org.sonar.check.Rule;
import org.sonar.plugins.communitydelphi.api.ast.ConstDeclarationNode;
import org.sonar.plugins.communitydelphi.api.ast.ConstSectionNode;
import org.sonar.plugins.communitydelphi.api.ast.DelphiNode;
import org.sonar.plugins.communitydelphi.api.ast.FieldSectionNode;
import org.sonar.plugins.communitydelphi.api.ast.MethodDeclarationNode;
import org.sonar.plugins.communitydelphi.api.ast.NameDeclarationNode;
import org.sonar.plugins.communitydelphi.api.ast.Node;
import org.sonar.plugins.communitydelphi.api.ast.StructTypeNode;
import org.sonar.plugins.communitydelphi.api.ast.TypeSectionNode;
import org.sonar.plugins.communitydelphi.api.ast.Visibility.VisibilityType;
import org.sonar.plugins.communitydelphi.api.ast.VisibilityNode;
import org.sonar.plugins.communitydelphi.api.ast.VisibilitySectionNode;
import org.sonar.plugins.communitydelphi.api.check.DelphiCheck;
import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext;
import org.sonar.plugins.communitydelphi.api.symbol.NameOccurrence;
import org.sonar.plugins.communitydelphi.api.symbol.scope.FileScope;
import org.sonarsource.analyzer.commons.annotations.DeprecatedRuleKey;

@DeprecatedRuleKey(ruleKey = "VisibilitySectionOrderRule", repositoryKey = "delph")
Expand All @@ -56,18 +70,98 @@ public DelphiCheckContext visit(StructTypeNode structTypeNode, DelphiCheckContex

private void checkOrder(
List<VisibilitySectionNode> visibilitySections, DelphiCheckContext context) {
int currentVisibilityOrder = VISIBILITY_ORDER.get(VisibilityType.IMPLICIT_PUBLISHED);
if (visibilitySections.size() < 2) {
return;
}

Deque<VisibilitySectionNode> pastSections = new ArrayDeque<>();
pastSections.add(visibilitySections.get(0));

for (int i = 1; i < visibilitySections.size(); i++) {
VisibilitySectionNode thisSection = visibilitySections.get(i);
VisibilitySectionNode lastSection = pastSections.peek();

for (var visibilitySection : visibilitySections) {
int sectionVisibilityOrder = VISIBILITY_ORDER.get(visibilitySection.getVisibility());
int thisSectionVisibility = VISIBILITY_ORDER.get(thisSection.getVisibility());

if (sectionVisibilityOrder >= currentVisibilityOrder) {
currentVisibilityOrder = sectionVisibilityOrder;
} else {
var visibilityNode = getVisibilityNode(visibilitySection);
reportIssue(
context, Objects.requireNonNullElse(visibilityNode, visibilitySection), MESSAGE);
// If this section has a lower visibility than the last section...
if (lastSection != null
&& VISIBILITY_ORDER.get(lastSection.getVisibility()) > thisSectionVisibility) {
// Remove excluded sections until a non-excluded one is found
while (lastSection != null && isExcludedSection(lastSection)) {
pastSections.pop();
lastSection = pastSections.peek();
}

// If the nearest non-excluded section is higher visibility, report an issue
if (lastSection != null
&& VISIBILITY_ORDER.get(lastSection.getVisibility()) > thisSectionVisibility) {
report(context, thisSection);
}
}

pastSections.add(thisSection);
}
}

private void report(DelphiCheckContext context, VisibilitySectionNode visibilitySection) {
var visibilityNode = getVisibilityNode(visibilitySection);
reportIssue(context, Objects.requireNonNullElse(visibilityNode, visibilitySection), MESSAGE);
}

private boolean isExcludedSection(VisibilitySectionNode visibilitySection) {
return visibilitySection.getChildren().stream().anyMatch(this::hasUsagesInsideType);
}

private boolean hasUsagesInsideType(DelphiNode node) {
StructTypeNode structNode = node.getFirstParentOfType(StructTypeNode.class);
return getVisibilitySectionItemUsages(node).stream()
.anyMatch(usage -> isInsideNode(structNode, usage.getLocation()));
}

private boolean isInsideNode(StructTypeNode structNode, Node node) {
if (!node.getScope()
.getEnclosingScope(FileScope.class)
.equals(structNode.getScope().getEnclosingScope(FileScope.class))) {
return false;
}

boolean afterStart =
structNode.getBeginLine() < node.getBeginLine()
|| (structNode.getBeginLine() == node.getBeginLine()
&& structNode.getBeginColumn() <= node.getBeginColumn());
boolean beforeEnd =
structNode.getEndLine() > node.getEndLine()
|| (structNode.getEndLine() == node.getEndLine()
&& structNode.getEndColumn() >= node.getEndColumn());

return afterStart && beforeEnd;
}

private List<NameOccurrence> getVisibilitySectionItemUsages(DelphiNode node) {
if (node instanceof MethodDeclarationNode) {
return ((MethodDeclarationNode) node).getMethodNameNode().getUsages();
} else if (node instanceof FieldSectionNode) {
return ((FieldSectionNode) node)
.getDeclarations().stream()
.flatMap(
declarationNode ->
declarationNode.getDeclarationList().getDeclarations().stream()
.map(NameDeclarationNode::getUsages)
.flatMap(Collection::stream))
.collect(Collectors.toList());
} else if (node instanceof ConstSectionNode) {
return node.findChildrenOfType(ConstDeclarationNode.class).stream()
.map(declarationNode -> declarationNode.getNameDeclarationNode().getUsages())
.flatMap(Collection::stream)
.collect(Collectors.toList());
} else if (node instanceof TypeSectionNode) {
return ((TypeSectionNode) node)
.getDeclarations().stream()
.map(typeDecl -> typeDecl.getTypeNameNode().getUsages())
.flatMap(Collection::stream)
.collect(Collectors.toList());
} else {
return Collections.emptyList();
}
}

Expand Down
18 changes: 18 additions & 0 deletions ...c/main/resources/org/sonar/l10n/delphi/rules/community-delphi/VisibilitySectionOrder.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,24 @@ <h2>Why is this an issue?</h2>
<li>public</li>
<li>published</li>
</ol>
<h3>Exceptions</h3>
<p>
This rule excludes cases where visibility sections declare a member that is used in later
visibility sections, such as the below:
</p>
<pre>
type
TMyType = class(TObject)
public
type
TMySubType = class(TObject);
end;
private
procedure DoSecretThing(Obj: TMySubType);
public
procedure DoPublicThing(Obj: TMySubType);
end;
</pre>
<h2>How to fix it</h2>
<p>Reorder the visibility sections within the type declaration into ascending order.</p>
<h2>Resources</h2>
Expand Down
145 changes: 144 additions & 1 deletion ...checks/src/test/java/au/com/integradev/delphi/checks/VisibilitySectionOrderCheckTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ void testMultipleOfTheSameSectionShouldNotAddIssue() {
new DelphiTestUnitBuilder()
.appendDecl("type")
.appendDecl(" TFoo = class(TObject)")
.appendDecl(" public // Noncompliant")
.appendDecl(" public")
.appendDecl(" procedure Bar;")
.appendDecl(" property Baz: Integer;")
.appendDecl(" public")
Expand Down Expand Up @@ -114,6 +114,44 @@ void testMultipleOutOfOrderSectionsShouldAddMultipleIssues() {
.verifyIssues();
}

@Test
void testMultipleReferencedOutOfOrderSectionsShouldNotAddIssue() {
CheckVerifier.newVerifier()
.withCheck(new VisibilitySectionOrderCheck())
.onFile(
new DelphiTestUnitBuilder()
.appendDecl("type")
.appendDecl(" TFoo = class(TObject)")
.appendDecl(" public")
.appendDecl(" type TBar = string;")
.appendDecl(" public")
.appendDecl(" const CBaz = 'abcd';")
.appendDecl(" private")
.appendDecl(" procedure Flarp(Arg: TBar = CBaz);")
.appendDecl(" end;"))
.verifyNoIssues();
}

@Test
void testOutOfOrderSectionAfterMultipleReferencedOutOfOrderSectionsShouldAddIssue() {
CheckVerifier.newVerifier()
.withCheck(new VisibilitySectionOrderCheck())
.onFile(
new DelphiTestUnitBuilder()
.appendDecl("type")
.appendDecl(" TFoo = class(TObject)")
.appendDecl(" protected")
.appendDecl(" FMyVar: Integer;")
.appendDecl(" public")
.appendDecl(" type TBar = string;")
.appendDecl(" public")
.appendDecl(" const CBaz = 'abcd';")
.appendDecl(" private // Noncompliant")
.appendDecl(" procedure Flarp(Arg: TBar = CBaz);")
.appendDecl(" end;"))
.verifyIssues();
}

@Test
void testImplicitPublishedFollowedByPrivateShouldNotAddIssue() {
CheckVerifier.newVerifier()
Expand All @@ -129,6 +167,111 @@ void testImplicitPublishedFollowedByPrivateShouldNotAddIssue() {
.verifyNoIssues();
}

@Test
void testOutOfOrderUnreferencedVisibilitySectionShouldAddIssue() {
CheckVerifier.newVerifier()
.withCheck(new VisibilitySectionOrderCheck())
.onFile(
new DelphiTestUnitBuilder()
.appendDecl("type")
.appendDecl(" TFoo = class(TObject)")
.appendDecl(" public")
.appendDecl(" const")
.appendDecl(" CMyConst = 'abcd';")
.appendDecl(" type")
.appendDecl(" TBar = TFlarp;")
.appendDecl(" FZorp: string;")
.appendDecl(" function GetMyInt: Integer;")
.appendDecl(" private // Noncompliant")
.appendDecl(" procedure Baz;")
.appendDecl(" end;"))
.verifyIssues();
}

@Test
void testOutOfOrderVisibilitySectionWithReferencedConstantShouldNotAddIssue() {
CheckVerifier.newVerifier()
.withCheck(new VisibilitySectionOrderCheck())
.onFile(
new DelphiTestUnitBuilder()
.appendDecl("type")
.appendDecl(" TFoo = class(TObject)")
.appendDecl(" public")
.appendDecl(" const")
.appendDecl(" CMyConst = 'abcd';")
.appendDecl(" type")
.appendDecl(" TBar = TFlarp;")
.appendDecl(" FZorp: string;")
.appendDecl(" function GetMyInt: Integer;")
.appendDecl(" private")
.appendDecl(" procedure Baz(MyArg: string = CMyConst);")
.appendDecl(" end;"))
.verifyNoIssues();
}

@Test
void testOutOfOrderVisibilitySectionWithReferencedTypeShouldNotAddIssue() {
CheckVerifier.newVerifier()
.withCheck(new VisibilitySectionOrderCheck())
.onFile(
new DelphiTestUnitBuilder()
.appendDecl("type")
.appendDecl(" TFoo = class(TObject)")
.appendDecl(" public")
.appendDecl(" const")
.appendDecl(" CMyConst = 'abcd';")
.appendDecl(" type")
.appendDecl(" TBar = TFlarp;")
.appendDecl(" FZorp: string;")
.appendDecl(" function GetMyInt: Integer;")
.appendDecl(" private")
.appendDecl(" procedure Baz(MyArg: TBar);")
.appendDecl(" end;"))
.verifyNoIssues();
}

@Test
void testOutOfOrderVisibilitySectionWithReferencedVarShouldNotAddIssue() {
CheckVerifier.newVerifier()
.withCheck(new VisibilitySectionOrderCheck())
.onFile(
new DelphiTestUnitBuilder()
.appendDecl("type")
.appendDecl(" TFoo = class(TObject)")
.appendDecl(" public")
.appendDecl(" const")
.appendDecl(" CMyConst = 'abcd';")
.appendDecl(" type")
.appendDecl(" TBar = TFlarp;")
.appendDecl(" FZorp: string;")
.appendDecl(" function GetMyInt: Integer;")
.appendDecl(" private")
.appendDecl(" property Baz: string read FZorp;")
.appendDecl(" end;"))
.verifyNoIssues();
}

@Test
void testOutOfOrderVisibilitySectionWithReferencedFunctionShouldNotAddIssue() {
CheckVerifier.newVerifier()
.withCheck(new VisibilitySectionOrderCheck())
.onFile(
new DelphiTestUnitBuilder()
.appendDecl("type")
.appendDecl(" TFoo = class(TObject)")
.appendDecl(" public")
.appendDecl(" const")
.appendDecl(" CMyConst = 'abcd';")
.appendDecl(" type")
.appendDecl(" TBar = TFlarp;")
.appendDecl(" FZorp: string;")
.appendDecl(" function GetMyInt: Integer;")
.appendDecl(" private")
.appendDecl(" property Baz: Integer read GetMyInt;")
.appendDecl(" end;"))
.verifyNoIssues();
}

@ParameterizedTest
@ValueSource(
strings = {
Expand Down

0 comments on commit 56a4d88

Please sign in to comment.