no error on missing defaut in non-final switches
UplinkCoder opened this issue · 40 comments
as per title.
We currently don't error when there is no default statement in a switch.
this causes strange code to be generated.
We should throw an error if there is no default case in a non-final switch.
I think this can be handle at parsing of switch statements. I would like to look into this.
I would not be wise to put a semantic restriction into the parser.
The error should be detected in the semantic phase.
@vivekvpandya please go ahead.
have a look at
libd/src/d/semantic/statement.d:502
@UplinkCoder yes, I realized that between switch and default there can be many case statements.
Which file contains semantic phase for switch ?? I can see lexer.d , parser.d etc. I am looking at dmd source for reference.
so the AstSwitchStatement has a statement inside it.
this statement is a blockStatement
you need to iterate trough the Statements inside of that blockStatement
and look for an AstLabeledStatement
where the statement.label == context.getName("default");
@vivekvpandya please feel free to ask for advice.
I just made a quick and dirty fix for that
auto cases = (cast(AstBlockStatement*)&s.statement).statements;
auto _default = context.getName("default");
import std.algorithm;
auto defaultCaseRange = cases
.map!(s => cast(AstLabeledStatement) s)
.filter!(s => s !is null)
.filter!(s => s.label == _default);
if (defaultCaseRange.empty) {
assert(0,"No default case!");
}
@vivekvpandya could you clean the above up ?
I have just started to change void visit(AstSwitchStatement s) method in SDC/libd/src/d/semantic/statement.d
it would be nice if the check could be done in a separate method
ok I will do it. and instead of assertion shouldn't we throw CompileException ??
yes.
we should generate an error and insert it in the flattenedStatements
that was what I meant with cleanup.
hmm it seems we have no ErrorStatement.
` bool checkForDefaultCase(AstSwitchStatement s){
bool defaultCaseStatus = true;
auto cases = (cast(AstBlockStatement*)&s.statement).statements;
auto _default = context.getName("default");
import std.algorithm;
auto defaultCaseRange = cases
.map!(s => cast(AstLabeledStatement) s)
.filter!(s => s !is null)
.filter!(s => s.label == _default);
if(defaultCaseRange.empty)
{
defaultCaseStatus = false;
}
return defaultCaseStatus;
}
void visit(AstSwitchStatement s) {
import d.semantic.expression;
if (!checkForDefaultCase(s)) {
throw new CompileException(s.location, "switch statement without a default; use 'final switch' or add 'default: assert(0);' or add 'default: break;'");
}
flattenedStmts ~= new SwitchStatement(
s.location,
ExpressionVisitor(pass).visit(s.expression),
autoBlock(s.statement),
);
}`
Is this look ok ?
so throwing a compile-Exception will be fine for now
use Egyptian-braces.
and
insert lineBreaks where you can
CompileException(s.location,
"switch statement without a default; use 'final switch' or add 'default: assert(0);' or add 'default: break;'"
);
checkForDefaultCase is a misleading name.
hasDefaultCase would be better.
on top of that we should check if there is more then one default case.
`bool hasDefaultCase(AstSwitchStatement s) {
bool defaultCaseStatus = true;
auto cases = (cast(AstBlockStatement*)&s.statement).statements;
auto _default = context.getName("default");
import std.algorithm;
auto defaultCaseRange = cases
.map!(s => cast(AstLabeledStatement) s)
.filter!(s => s !is null)
.filter!(s => s.label == _default);
if(defaultCaseRange.empty) {
defaultCaseStatus = false;
}
return defaultCaseStatus;
}
void visit(AstSwitchStatement s) {
import d.semantic.expression;
if (!hasDefaultCase(s)) {
throw new CompileException(s.location,
"switch statement without a default; use 'final switch' or add 'default: assert(0);' or add 'default: break;'");
}
flattenedStmts ~= new SwitchStatement(
s.location,
ExpressionVisitor(pass).visit(s.expression),
autoBlock(s.statement),
);
}`
it would be better to ask for hasNoDefaultCase... because then you don't need to init the variable explicitly.
when the range in non-empty you should do a popFront and check if it's empty now.
if it's not you need to throw a diffrent error.
so better throw the error in the check function.
then the check function can have the signature void check(Whatever)
and we can automatise doing the checks in a different thread.
void hasDefaultCase(AstSwitchStatement s) {
auto cases = (cast(AstBlockStatement*)&s.statement).statements;
auto _default = context.getName("default");
import std.algorithm;
auto defaultCaseRange = cases
.map!(s => cast(AstLabeledStatement) s)
.filter!(s => s !is null)
.filter!(s => s.label == _default);
if(defaultCaseRange.empty) {
throw new CompileException(s.location,
"switch statement without a default; use 'final switch' or add 'default: assert(0);' or add 'default: break;'");
}
else if {
defaultCaseRange.popFront();
if(!defaultCaseRange.empty) {
// multiple default: cases
throw new CompileException(s.location,
"switch statement already has a default");
}
}
}
void visit(AstSwitchStatement s) {
import d.semantic.expression;
hasDefaultCase(s);
flattenedStmts ~= new SwitchStatement(
s.location,
ExpressionVisitor(pass).visit(s.expression),
autoBlock(s.statement),
);
}
change the error message to something more meaningful please.
and please rename the function to check
then you can open a PR.
Actually error message is copied from dmd
function name only check or checkHasDefaultCase ??
we try to improve on dmd here.
only check
ok, why only check ??
because it should be the same name for all semantic checks.
we can the automatically dispatch. the the same as with the visitors.
throw new CompileException(s.location,
"switch statement with multiple defaults are not allowed ");
Is this ok ?
"switch statements with multiple defaults are not allowed"
LGTM please open a PR.
void check(AstSwitchStatement s) {
auto cases = (cast(AstBlockStatement*)&s.statement).statements;
auto _default = context.getName("default");
import std.algorithm;
auto defaultCaseRange = cases
.map!(s => cast(AstLabeledStatement) s)
.filter!(s => s !is null)
.filter!(s => s.label == _default);
if(defaultCaseRange.empty) {
throw new CompileException(s.location,
"switch statement without a default; use 'final switch' or add 'default: assert(0);' or add 'default: break;'");
}
else if {
defaultCaseRange.popFront();
if(!defaultCaseRange.empty) {
// multiple default: cases
throw new CompileException(s.location,
"switch statement with multiple defaults are not allowed ");
}
}
}
void visit(AstSwitchStatement s) {
import d.semantic.expression;
check(s);
flattenedStmts ~= new SwitchStatement(
s.location,
ExpressionVisitor(pass).visit(s.expression),
autoBlock(s.statement),
);
}
finally it looks like this
I'll comment on the PR.
and please have a look at my code above and add newlines accordingly.
this is fixed by #193
No this isn't fixed by this PR and the whole suggested approach is wrong.
You can fix this by adding a flag in FlowAnalyzer for the presence of a default case. Save the state of the flag at switch entrance. Set the flag when encountering a default case. Check the flag on switch exit and restore the previous value.
Add flag here: https://github.com/SDC-Developers/SDC/blob/master/libd/src/d/semantic/flow.d#L37
Save/Restore here: https://github.com/SDC-Developers/SDC/blob/master/libd/src/d/semantic/flow.d#L193-L210
Check flag here: https://github.com/SDC-Developers/SDC/blob/master/libd/src/d/semantic/flow.d#L218
Set flag here: https://github.com/SDC-Developers/SDC/blob/master/libd/src/d/semantic/flow.d#L321
Don't forget to add a test case.
@vivekvpandya can you handle this ?
I'll give advice when asked.
@UplinkCoder yes let me give it a try.
I will take some more time than regular D developer because I also needs to understand various D specific features.
@UplinkCoder @deadalnix this should be fixed by PR #199
Yes, closing.