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

Attempts to fix issue #266 #317

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Attempts to fix issue #266 #317

wants to merge 8 commits into from

Conversation

keyehzy
Copy link
Contributor

@keyehzy keyehzy commented May 18, 2021

We do this by treating else ( separately. If else is followed by
a (conditional + block) pattern, maybe the intention was to use an
else if instead and a warning is emitted.

We do this by treating `else (` separately. If `else` is followed by
a (conditional + block) pattern, maybe the intention was to use an
`else if` instead and a warning is emitted.
src/quick-lint-js/error.h Outdated Show resolved Hide resolved
test/test-parse-statement.cpp Show resolved Hide resolved
src/quick-lint-js/parse.h Show resolved Hide resolved
src/quick-lint-js/parse.h Show resolved Hide resolved
Included function to check if the expression is side-effect-free,
although I have to check more carefully if the cases are done
correctly.

The cases that explicitly not side-effect-free are: dot, index,
function and named_functions.

From testing I found that arrow functions invalidate placing an `if`,
even if their children are side-effect-free. This case is treated
seperately. Maybe there are more cases where this happens and I must
think it through.
Copy link
Collaborator

@strager strager left a comment

Choose a reason for hiding this comment

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

Hmm, this issue is more complicated than I thought! Lots of edge cases.

src/quick-lint-js/parse.h Outdated Show resolved Hide resolved
src/quick-lint-js/parse.h Outdated Show resolved Hide resolved
src/quick-lint-js/parse.h Outdated Show resolved Hide resolved
src/quick-lint-js/parse.h Outdated Show resolved Hide resolved
src/quick-lint-js/parse.h Outdated Show resolved Hide resolved
src/quick-lint-js/parse.h Outdated Show resolved Hide resolved
test/test-parse-statement.cpp Outdated Show resolved Hide resolved
src/quick-lint-js/parse.h Show resolved Hide resolved
test/test-parse-statement.cpp Show resolved Hide resolved
test/test-parse-statement.cpp Show resolved Hide resolved
This function has been refactored and tested for edge cases.
src/parse.cpp Outdated
auto entry = ast->object_entry(i);
if (entry.property.has_value()) {
if (this->has_potential_side_effects(*entry.property) ||
this->has_potential_side_effects(entry.value))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we check entry.value regardless of entry.property.has_value()?

test/test-parse-expression.cpp Outdated Show resolved Hide resolved
}

{
test_parser p(u8"import foo;"_sv);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocking: This has syntax errors. I think you meant this:

import(foo);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is resolved.

test/test-parse-expression.cpp Outdated Show resolved Hide resolved
test/test-parse-expression.cpp Show resolved Hide resolved
test/test-parse-expression.cpp Outdated Show resolved Hide resolved
test/test-parse-expression.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@strager strager left a comment

Choose a reason for hiding this comment

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

Blocking: if (a) { b; } else (c) { d; } is still an issue (false negative).

Other than that, this commit looks bueno!

TEST(test_parse, else_with_implicit_if) {
{
spy_visitor v;
padded_string code(u8"if (a) { b; } else (c)\n{ d; }"_sv);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should check v.visits so we know we recovered from the error correctly.

test/test-parse-statement.cpp Outdated Show resolved Hide resolved
src/parse.cpp Outdated
auto entry = ast->object_entry(i);
if (entry.property.has_value()) {
if (has_potential_side_effects(*entry.property) ||
has_potential_side_effects(entry.value))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we check entry.value regardless of entry.property.has_value()?

src/parse.cpp Outdated Show resolved Hide resolved
break;
}

this->consume_semicolon();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocking: This looks like it's in the wrong spot. Won't skipping a semicolon here cause the following code to have the error_else_with_conditional_missing_if warning?

if (a) { b; } else (c);
{ d; }

I think we need to consume a semicolon after the check on line 2546.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a test for this example.

test/test-parse-statement.cpp Show resolved Hide resolved
Comment on lines +2124 to +2125
return has_potential_side_effects(*entry.property) ||
has_potential_side_effects(entry.value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocking: You're only checking the first entry which has a value! We should check all entries.

break;
}

this->consume_semicolon();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a test for this example.

@strager
Copy link
Collaborator

strager commented Jan 5, 2022

There are merge conflicts because parser::parse_and_visit_if's body has been moved to src/quick-lint-js/parse-statement-inl.h.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants