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

Prevent unused variable warnings on ternary and null coalescence expressions #1101

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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: 8 additions & 6 deletions src/parser/Expression.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1393,8 +1393,9 @@ export class TernaryExpression extends Expression {

transpile(state: BrsTranspileState) {
let result = [] as TranspileResult;
let consequentInfo = util.getExpressionInfo(this.consequent!);
let alternateInfo = util.getExpressionInfo(this.alternate!);
const file = state.file;
let consequentInfo = util.getExpressionInfo(this.consequent!, file);
let alternateInfo = util.getExpressionInfo(this.alternate!, file);

//get all unique variable names used in the consequent and alternate, and sort them alphabetically so the output is consistent
let allUniqueVarNames = [...new Set([...consequentInfo.uniqueVarNames, ...alternateInfo.uniqueVarNames])].sort();
Expand All @@ -1409,7 +1410,7 @@ export class TernaryExpression extends Expression {
this.questionMarkToken,
//write all the scope variables as parameters.
//TODO handle when there are more than 31 parameters
`(function(__bsCondition, ${allUniqueVarNames.join(', ')})`
`(function(${['__bsCondition', ...allUniqueVarNames].join(', ')})`
),
state.newline,
//double indent so our `end function` line is still indented one at the end
Expand All @@ -1433,7 +1434,7 @@ export class TernaryExpression extends Expression {
state.indent(-1),
state.sourceNode(this.questionMarkToken, 'end function)('),
...this.test.transpile(state),
state.sourceNode(this.questionMarkToken, `, ${allUniqueVarNames.join(', ')})`)
state.sourceNode(this.questionMarkToken, `${['', ...allUniqueVarNames].join(', ')})`)
);
state.blockDepth--;
} else {
Expand Down Expand Up @@ -1476,8 +1477,9 @@ export class NullCoalescingExpression extends Expression {

transpile(state: BrsTranspileState) {
let result = [] as TranspileResult;
let consequentInfo = util.getExpressionInfo(this.consequent);
let alternateInfo = util.getExpressionInfo(this.alternate);
const file = state.file;
let consequentInfo = util.getExpressionInfo(this.consequent, file);
let alternateInfo = util.getExpressionInfo(this.alternate, file);
TwitchBronBron marked this conversation as resolved.
Show resolved Hide resolved

//get all unique variable names used in the consequent and alternate, and sort them alphabetically so the output is consistent
let allUniqueVarNames = [...new Set([...consequentInfo.uniqueVarNames, ...alternateInfo.uniqueVarNames])].sort();
Expand Down
46 changes: 46 additions & 0 deletions src/parser/tests/expression/NullCoalescenceExpression.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -280,5 +280,51 @@ describe('NullCoalescingExpression', () => {
end sub
`);
});

it('ignores enum variable names', () => {
testTranspile(`
enum Direction
up = "up"
end enum
sub main()
d = invalid
a = d ?? Direction.up
end sub
`, `
sub main()
d = invalid
a = (function(d)
__bsConsequent = d
if __bsConsequent <> invalid then
return __bsConsequent
else
return "up"
end if
end function)(d)
end sub
`);
});

it('ignores const variable names', () => {
testTranspile(`
const USER = "user"
sub main()
settings = {}
a = m.defaults.getAccount(settings.name) ?? USER
end sub
`, `
sub main()
settings = {}
a = (function(m, settings)
__bsConsequent = m.defaults.getAccount(settings.name)
if __bsConsequent <> invalid then
return __bsConsequent
else
return "user"
end if
end function)(m, settings)
end sub
`);
});
});
});
49 changes: 49 additions & 0 deletions src/parser/tests/expression/TernaryExpression.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,55 @@ describe('ternary expressions', () => {
`);
});

it('ignores enum variable names', () => {
testTranspile(`
enum Direction
up = "up"
down = "down"
end enum
sub main()
d = Direction.up
theDir = d = Direction.up ? Direction.up : false
end sub
`, `
sub main()
d = "up"
theDir = (function(__bsCondition)
if __bsCondition then
return "up"
else
return false
end if
end function)(d = "up")
end sub
`);
});

it('ignores const variable names', () => {
testTranspile(`
enum Direction
up = "up"
down = "down"
end enum
const UP = "up"
sub main()
d = Direction.up
theDir = d = Direction.up ? UP : Direction.down
end sub
`, `
sub main()
d = "up"
theDir = (function(__bsCondition)
if __bsCondition then
return "up"
else
return "down"
end if
end function)(d = "up")
end sub
`);
});

it('supports scope-captured outer, and simple inner', () => {
testTranspile(
`
Expand Down
12 changes: 10 additions & 2 deletions src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1189,7 +1189,7 @@ export class Util {
* Gathers expressions, variables, and unique names from an expression.
* This is mostly used for the ternary expression
*/
public getExpressionInfo(expression: Expression): ExpressionInfo {
public getExpressionInfo(expression: Expression, file: BrsFile): ExpressionInfo {
const expressions = [expression];
const variableExpressions = [] as VariableExpression[];
const uniqueVarNames = new Set<string>();
Expand All @@ -1213,7 +1213,15 @@ export class Util {
//handle the expression itself (for situations when expression is a VariableExpression)
expressionWalker(expression);

return { expressions: expressions, varExpressions: variableExpressions, uniqueVarNames: [...uniqueVarNames] };
const scope = file.program.getFirstScopeForFile(file);
const filteredVarNames = [...uniqueVarNames].filter((varName: string) => {
const varNameLower = varName.toLowerCase();
// TODO: include namespaces in this filter
return !scope.getEnumMap().has(varNameLower) &&
!scope.getConstMap().has(varNameLower);
});

return { expressions: expressions, varExpressions: variableExpressions, uniqueVarNames: filteredVarNames };
}


Expand Down