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

Code action callfunc operator refactor #385

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/CodeActionUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ export class CodeActionUtil {
TextEdit.insert(change.position, change.newText)
);
} else if (change.type === 'replace') {
TextEdit.replace(change.range, change.newText);
edit.changes[uri].push(
TextEdit.replace(change.range, change.newText)
);
} else if (change.type === 'delete') {
TextEdit.del(change.range);
}
}
return CodeAction.create(obj.title, edit);
Expand All @@ -32,7 +36,7 @@ export interface CodeActionShorthand {
diagnostics?: Diagnostic[];
kind?: CodeActionKind;
isPreferred?: boolean;
changes: Array<InsertChange | ReplaceChange>;
changes: Array<InsertChange | ReplaceChange | DeleteChange>;
}

export interface InsertChange {
Expand All @@ -49,4 +53,10 @@ export interface ReplaceChange {
range: Range;
}

export interface DeleteChange {
filePath: string;
type: 'delete';
range: Range;
}

export const codeActionUtil = new CodeActionUtil();
78 changes: 78 additions & 0 deletions src/bscPlugin/codeActions/CodeActionsProcessor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,84 @@ describe('CodeActionsProcessor', () => {
`import "pkg:/source/Animals.bs"`
]);
});

it('suggests callfunc operator refactor (parameterless)', () => {
const file = program.addOrReplaceFile('components/main.bs', `
sub doSomething()
someComponent.callfunc("doThing")
Copy link
Contributor

Choose a reason for hiding this comment

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

nobody would actually have this code, coz it would not work (it woudl do nothing at run time).

It would have to be someComponent.callfunc("doThing", invalid), which (imho) should become someComponent@.doThing() (which is semantically equivalent).

Copy link
Member Author

@TwitchBronBron TwitchBronBron Apr 9, 2021

Choose a reason for hiding this comment

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

While I understand the "standard", I've seen callfunc in the wild with no second parameter. I believe @elsassph also had mentioned they use it like that in their code successfully.

However, I agree, we should refactor someComponent.callfunc("doThing", invalid) to someComponent@.doThing() as well since I think that's what we do behind the scenes in the transpiled BrightScript, so I'll update this PR to support that as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

callfunc without argument should be a warning, but I have seen it.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, guess you guys are right, if there's a default param on the function, then in that case callfunc without invalid will work (I wasn't aware of this or it changed since I got this notion > 2 years ago). Clearly, we can never know if this is the case, so 🤷 I'm not sure if even have a warning here..

end sub
`);
program.validate();

expectCodeActions(() => {
program.getCodeActions(
file.pathAbsolute,
util.createRange(2, 39, 2, 39)
);
}, [{
title: `Refactor to use callfunc operator`,
isPreferred: false,
kind: 'quickfix',
changes: [{
filePath: file.pathAbsolute,
newText: '@.doThing(',
type: 'replace',
range: util.createRange(2, 33, 2, 52)
}]
}]);
});

it('suggests callfunc operator refactor (parameters)', () => {
const file = program.addOrReplaceFile('components/main.bs', `
sub doSomething()
someComponent.callfunc("doOtherThing", someIdentifier, 2)
end sub
`);
program.validate();

expectCodeActions(() => {
program.getCodeActions(
file.pathAbsolute,
util.createRange(2, 39, 2, 39)
);
}, [{
title: `Refactor to use callfunc operator`,
isPreferred: false,
kind: 'quickfix',
changes: [{
filePath: file.pathAbsolute,
newText: '@.doOtherThing(',
type: 'replace',
range: util.createRange(2, 33, 2, 59)
}]
}]);
});

it('suggests callfunc operator refactor (remove "invalid" first param)', () => {
const file = program.addOrReplaceFile('components/main.bs', `
sub doSomething()
someComponent.callfunc("doYetAnotherThing", invalid)
end sub
`);
program.validate();

expectCodeActions(() => {
program.getCodeActions(
file.pathAbsolute,
util.createRange(2, 39, 2, 39)
);
}, [{
title: `Refactor to use callfunc operator`,
isPreferred: false,
kind: 'quickfix',
changes: [{
filePath: file.pathAbsolute,
newText: '@.doYetAnotherThing(',
type: 'replace',
range: util.createRange(2, 33, 2, 71)
}]
}]);
});
});

});
58 changes: 57 additions & 1 deletion src/bscPlugin/codeActions/CodeActionsProcessor.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import type { Diagnostic } from 'vscode-languageserver';
import type { Diagnostic, Position } from 'vscode-languageserver';
import { CodeActionKind } from 'vscode-languageserver';
import { isBrsFile } from '../../astUtils';
import { codeActionUtil } from '../../CodeActionUtil';
import type { DiagnosticMessageType } from '../../DiagnosticMessages';
import { DiagnosticCodeMap } from '../../DiagnosticMessages';
import type { BrsFile } from '../../files/BrsFile';
import type { XmlFile } from '../../files/XmlFile';
import type { BscFile, OnGetCodeActionsEvent } from '../../interfaces';
import type { Token } from '../../lexer/Token';
import { TokenKind } from '../../lexer/TokenKind';
import { ParseMode } from '../../parser';
import { util } from '../../util';

Expand All @@ -26,6 +29,59 @@ export class CodeActionsProcessor {
this.addMissingExtends(diagnostic as any);
}
}

if (isBrsFile(this.event.file)) {
const token = this.event.file.getTokenAt(this.event.range.start);
const previousToken = this.event.file.getPreviousToken(token);
const lowerText = token?.text?.toLowerCase();

//brighterscript-specific code actions
if (this.event.file.extension === '.bs') {
if (lowerText === 'callfunc' && previousToken.kind === TokenKind.Dot) {
this.suggestCallfuncOperator(token, previousToken);
}
}
}
}

/**
* Suggest refactoring callfunc calls to use the callfunc operator instead
*/
private suggestCallfuncOperator(token: Token, previousToken: Token) {
const file = this.event.file as BrsFile;
const openParenToken = file.getNextToken(token);
const functionNameToken = file.getNextToken(openParenToken);

const comma = file.getNextToken(functionNameToken, TokenKind.Comma);
const invalidLiteral = file.getNextToken(comma, TokenKind.Invalid);
let endPosition: Position;
const endToken = invalidLiteral ?? comma;
//consume the comma and any space up until the next token
if (endToken) {
const tokenAfter = file.getNextToken(endToken);
endPosition = tokenAfter?.range.start ?? comma?.range.end;
} else {
endPosition = functionNameToken.range.end;
}

//if we have the necessary tokens
if (openParenToken?.kind === TokenKind.LeftParen && functionNameToken?.kind === TokenKind.StringLiteral) {
//replace leading and trailing quotes
const functionName = functionNameToken.text.replace(/^"/, '').replace(/"$/, '');
this.event.codeActions.push(
codeActionUtil.createCodeAction({
title: `Refactor to use callfunc operator`,
isPreferred: false,
kind: CodeActionKind.QuickFix,
changes: [{
type: 'replace',
filePath: this.event.file.pathAbsolute,
newText: `@.${functionName}(`,
range: util.createRangeFromPositions(previousToken.range.start, endPosition)
}]
})
);
}
}

private suggestedImports = new Set<string>();
Expand Down
15 changes: 15 additions & 0 deletions src/files/BrsFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1092,6 +1092,21 @@ export class BrsFile {
return parser.tokens[idx - 1];
}

/**
* Get the token after the given token.
* If `requiredTokenKind` is specified, then the next token's type is checked, and if no match, undefined is returned.
*/
public getNextToken(token: Token, requiredTokenKind?: TokenKind) {
const parser = this.parser;
let idx = parser.tokens.indexOf(token);
const result = parser.tokens[idx + 1];
if (!requiredTokenKind) {
return result;
} else if (result?.kind === requiredTokenKind) {
return result;
}
}

/**
* Find the first scope that has a namespace with this name.
* Returns false if no namespace was found with that name
Expand Down