Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEW (Extension) @W-13991634@ Add class level PMD violations #91

Merged
merged 10 commits into from
Jun 10, 2024
25 changes: 25 additions & 0 deletions code-fixtures/fixer-tests/MyClass2.cls
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright (c) 2023, Salesforce, Inc.
* All rights reserved.
* SPDX-License-Identifier: BSD-3-Clause
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/
public class MyClass2 {

public static boolean someBooleanMethod() {
// some comment that includes public class MyClass2 {
return false;
}
/* some other comment in a single line */
public static boolean someOtherBooleanMethod() {
/*
some other comment that includes public class MyClass 2 {
*/
return false;
}

public static boolean someOtherMethod() {
public static String someString = 'this string has class MyClass2 { ';
return true;
}
}
4 changes: 2 additions & 2 deletions src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,9 @@ export async function activate(context: vscode.ExtensionContext): Promise<vscode
outputChannel
});
});
const removeDiagnosticsOnSelectedFile = vscode.commands.registerCommand(Constants.COMMAND_REMOVE_DIAGNOSTRICS_ON_SELECTED_FILE, async (selection: vscode.Uri, multiSelect?: vscode.Uri[]) => {
const removeDiagnosticsOnSelectedFile = vscode.commands.registerCommand(Constants.COMMAND_REMOVE_DIAGNOSTICS_ON_SELECTED_FILE, async (selection: vscode.Uri, multiSelect?: vscode.Uri[]) => {
return _clearDiagnosticsForSelectedFiles(multiSelect && multiSelect.length > 0 ? multiSelect : [selection], {
commandName: Constants.COMMAND_REMOVE_DIAGNOSTRICS_ON_SELECTED_FILE,
commandName: Constants.COMMAND_REMOVE_DIAGNOSTICS_ON_SELECTED_FILE,
diagnosticCollection,
outputChannel
});
Expand Down
2 changes: 1 addition & 1 deletion src/lib/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export const COMMAND_RUN_ON_ACTIVE_FILE = 'sfca.runOnActiveFile';
export const COMMAND_RUN_ON_SELECTED = 'sfca.runOnSelected';
export const COMMAND_RUN_DFA_ON_SELECTED_METHOD = 'sfca.runDfaOnSelectedMethod';
export const COMMAND_REMOVE_DIAGNOSTICS_ON_ACTIVE_FILE = 'sfca.removeDiagnosticsOnActiveFile';
export const COMMAND_REMOVE_DIAGNOSTRICS_ON_SELECTED_FILE = 'sfca.removeDiagnosticsOnSelectedFile';
export const COMMAND_REMOVE_DIAGNOSTICS_ON_SELECTED_FILE = 'sfca.removeDiagnosticsOnSelectedFile';
export const COMMAND_REMOVE_SINGLE_DIAGNOSTIC = 'sfca.removeSingleDiagnostic'


Expand Down
165 changes: 165 additions & 0 deletions src/lib/fixer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,12 @@ export class _NoOpFixGenerator extends FixGenerator {
* @private Must be exported for testing purposes, but shouldn't be used publicly, hence the leading underscore.
*/
export class _PmdFixGenerator extends FixGenerator {
private singleLineCommentPattern = /^\s*\/\//;
private blockCommentStartPattern = /^\s*\/\*/;
private blockCommentEndPattern = /\*\//;
private classDeclarationPattern = /\b(\w+\s+)+class\s+\w+/;
public suppressionRegex = /@SuppressWarnings\s*\(\s*'([^']*)'\s*\)/;

/**
* Generate an array of fixes, if possible.
* @returns
Expand All @@ -98,6 +104,7 @@ export class _PmdFixGenerator extends FixGenerator {
const fixes: vscode.CodeAction[] = [];
if (this.documentSupportsLineLevelSuppression()) {
fixes.push(this.generateLineLevelSuppression());
fixes.push(this.generateClassLevelSuppression());
}
return fixes;
}
Expand Down Expand Up @@ -131,6 +138,164 @@ export class _PmdFixGenerator extends FixGenerator {
title: 'Clear Single Diagnostic',
arguments: [this.document.uri, this.diagnostic]
};

return action;
}

public generateClassLevelSuppression(): vscode.CodeAction {
// Find the end-of-line position of the class declaration where the diagnostic is found.
const classStartPosition = this.findClassStartPosition(this.diagnostic, this.document);
// const classEndOfLinePosition = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove commented out code.


const action = new vscode.CodeAction(messages.fixer.supressOnClass, vscode.CodeActionKind.QuickFix);
action.edit = new vscode.WorkspaceEdit();

// Determine the appropriate suppression rule based on the type of diagnostic.code
let suppressionRule: string;
if (typeof this.diagnostic.code === 'object' && this.diagnostic.code !== null && 'value' in this.diagnostic.code) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need both the === 'object' and !== null check? Seems like the first would be sufficient. Also, in JS/TS it's typical to do == null instead of === null, because undefined == null and usually you want to catch both null and undefined.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

suppressionRule = `PMD.${this.diagnostic.code.value}`;
} else {
suppressionRule = `PMD`;
}

// Extract text from the start to end of the class declaration to search for existing suppressions
const classText = this.findLineBeforeClassStartDeclaration(classStartPosition, this.document);
const suppressionMatch = classText.match(this.suppressionRegex);

if (suppressionMatch) {
// If @SuppressWarnings exists, check if the rule is already present
const existingRules = suppressionMatch[1].split(',').map(rule => rule.trim());
if (!existingRules.includes(suppressionRule)) {
Comment on lines +174 to +175
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering about case sensitivity of the rules. If they are case insensitive, then we probably should add to the map a .toLowerCase() and then add it to the suppressionRule when checking for existence.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up question: Is it an error case if the rule is already present, or is it valid and nothing should happen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added case insensitivity in the regex and added tests. It is an error case when a rule is already present and violations are present. One scenario I can think of is when user ran the scanner without the rule and manually added the rule and then tried to run the suppress at the class level. I'm not handling that part.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding case insensitivity in the regex doesn't help with this line does it existingRules.includes(suppressionRule)) ?

I'd recommend something like
const existingRules = suppressionMatch[1].split(',').map(rule => rule.trim().toLowerCase());
if (!existingRules.includes(suppressionRule.toLowerCase())) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, this has an unintended side effect. It changes all existing rules to lower case since we concatenate the existing rules (which were made to be lower case for comparison) to new rules. So, we decided to leave this as is.

// If the rule is not present, add it to the existing @SuppressWarnings
const updatedRules = [...existingRules, suppressionRule].join(', ');
const updatedSuppression = this.generateUpdatedSuppressionTag(updatedRules, this.document.languageId);
const suppressionStartPosition = this.document.positionAt(classText.indexOf(suppressionMatch[0]));
const suppressionEndPosition = this.document.positionAt(classText.indexOf(suppressionMatch[0]) + suppressionMatch[0].length);
const suppressionRange = new vscode.Range(suppressionStartPosition, suppressionEndPosition);
action.edit.replace(this.document.uri, suppressionRange, updatedSuppression);
}
} else {
// If @SuppressWarnings does not exist, insert a new one
const newSuppression = this.generateNewSuppressionTag(suppressionRule, this.document.languageId);
action.edit.insert(this.document.uri, classStartPosition, newSuppression);
}

action.diagnostics = [this.diagnostic];
action.command = {
command: Constants.COMMAND_REMOVE_DIAGNOSTICS_ON_SELECTED_FILE,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I clear the diagnostics for the whole file and this is not ideal when a file has multiple classes. Keeping track of the diagnostics that would be affected by the rule added on the specific class seemed too difficult and the AC for the story did not call for it. Since this PR is already doing too many things, I could still do it as part of a separate story.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarification: Does "multiple classes" mean multiple outer classes, or an outer class with inner classes? I know Apex can't have multiple outer classes, and I'm pretty sure that the annotation will only apply to outer classes, though I haven't checked that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't realize apex can't have multiple outer classes. I have changed the code and now this applies only to the outer class.

title: 'Remove diagnostics for this file',
arguments: [this.document.uri]
};

return action;
}

public generateUpdatedSuppressionTag(updatedRules: string, lang: string) {
if (lang === 'apex') {
return `@SuppressWarnings('${updatedRules}')`;
} else if (lang === 'java') {
return `@SuppressWarnings("${updatedRules}")`;
}
return '';
}

public generateNewSuppressionTag(suppressionRule: string, lang: string) {
if (lang === 'apex') {
return `@SuppressWarnings('${suppressionRule}')\n`;
} else if (lang === 'java') {
return `@SuppressWarnings("${suppressionRule}")\n`;
}
return '';
}

/**
* Finds the start position of the class in the document.
* Assumes that the class declaration starts with the keyword "class".
* @returns The position at the start of the class.
*/
public findClassStartPosition(diagnostic: vscode.Diagnostic, document: vscode.TextDocument): vscode.Position {
Comment on lines +218 to +223
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was confused at first by this... I didn't realize until I read the code that you always return column 0. This sounds like it is the position (line, col) exactly where you find the word "class". But in reality you are finding the starting line of the class and returning the zeroth column. I recommend you update your comment to make this clear. Otherwise, I was scratching my head earlier in the code wondering how you could put a newline character immediately before the word "class".

const text = document.getText();
const diagnosticLine = diagnostic.range.start.line;

// Split the text into lines for easier processing
const lines = text.split('\n');
let classStartLine: number | undefined;

let inBlockComment = false;

// Iterate from the diagnostic line upwards to find the class declaration
for (let lineNumber = diagnosticLine; lineNumber >= 0; lineNumber--) {
const line = lines[lineNumber];

// Check if we are in a block comment
if (inBlockComment) {
if (line.match(this.blockCommentStartPattern)) {
inBlockComment = false;
}
continue;
}

// Skip single-line comments
if (line.match(this.singleLineCommentPattern)) {
continue;
}

// Skip block comment in a single line
if (line.match(this.blockCommentEndPattern) && line.match(this.blockCommentStartPattern)) {
continue;
}

// Check if this line is the start of a block comment
if (line.match(this.blockCommentEndPattern)) {
inBlockComment = true;
continue;
}
jag-j marked this conversation as resolved.
Show resolved Hide resolved

const match = line.match(this.classDeclarationPattern);
if (!inBlockComment && match && !this.isWithinQuotes(line, match.index)) {
classStartLine = lineNumber;
break;
}
}

if (classStartLine !== undefined) {
return new vscode.Position(classStartLine, 0);
}

// Default to the start of the document if class is not found
return new vscode.Position(0, 0);
}

/**
* Finds the entire line that is one line above a class declaration statement.
* @returns The text of the line that is one line above the class declaration.
*/
public findLineBeforeClassStartDeclaration(classStartPosition: vscode.Position, document: vscode.TextDocument): string {
// Ensure that there is a line before the class declaration
if (classStartPosition.line > 0) {
const lineBeforeClassPosition = classStartPosition.line - 1;
const lineBeforeClass = document.lineAt(lineBeforeClassPosition);
return lineBeforeClass.text;
}

// Return an empty string if it's the first line of the document
return '';
}

/**
* Helper function to check if match is within quotes
* @param line
* @param matchIndex
* @returns
*/
public isWithinQuotes(line: string, matchIndex: number): boolean {
const beforeMatch = line.slice(0, matchIndex);
const singleQuotesBefore = (beforeMatch.match(/'/g) || []).length;
const doubleQuotesBefore = (beforeMatch.match(/"/g) || []).length;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does apex allow double quotes? I thought strings must be single quoted? Not sure if you need to worry about double quotes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strings have to be single-quoted. I genuinely don't remember if you're allowed to use double-quotes as string boundaries.
BUT you might want to check whether the string containing escaped single-quotes throws off the algorithm. E.g., 'this is a string. it\'s got a single-quote inside it'.


// Check if the number of quotes before the match is odd (inside quotes)
if (singleQuotesBefore % 2 !== 0 || doubleQuotesBefore % 2 !== 0) return true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pet peeve: I'm broadly opposed to single-line/braceless ifs.


return false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lower golf score:
return <conditional_statement>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
}
3 changes: 2 additions & 1 deletion src/lib/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ export const messages = {
existingDfaRunText: "A Salesforce Graph Engine analysis is already running. Cancel it by clicking in the Status Bar.",
},
fixer: {
supressOnLine: "Suppress violations on this line."
supressOnLine: "Suppress violations on this line.",
supressOnClass: "***Suppress violations on this class.***"
},
diagnostics: {
messageGenerator: (severity: number, message: string) => `Sev${severity}: ${message}`,
Expand Down
Loading