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

10$: Warn on if("x"||"y"), case "x"||"y":, etc. #953

Open
strager opened this issue Mar 19, 2023 · 8 comments · May be fixed by #993
Open

10$: Warn on if("x"||"y"), case "x"||"y":, etc. #953

strager opened this issue Mar 19, 2023 · 8 comments · May be fixed by #993
Labels
for hire Get paid for working on this task: https://quick-lint-js.com/hiring.html good first issue Good for newcomers and C++ beginners

Comments

@strager
Copy link
Collaborator

strager commented Mar 19, 2023

if ("x" || "y") { // expect: diagnostic
}

switch (s) {
  case "x" || "y": // expect: diagnostic
    break;
}
@strager strager added good first issue Good for newcomers and C++ beginners for hire Get paid for working on this task: https://quick-lint-js.com/hiring.html labels Mar 19, 2023
@strager strager changed the title 10$: Warn on if(x"||"y"), case "x"||"y":, etc. 10$: Warn on if("x"||"y"), case "x"||"y":, etc. Mar 19, 2023
@ybl3
Copy link

ybl3 commented Mar 29, 2023

Could I ask what coding language this is expected to be done in?

@strager
Copy link
Collaborator Author

strager commented Mar 29, 2023

@ybl3 C++.

The patch would probably look like this:

  • Add a new diagnostic type. See docs: https://quick-lint-js.com/contribute/create-diagnostic/
  • Add a function in src/quick-lint-js/fe/parse.cpp similar to parser::error_on_sketchy_condition. Call this function as appropriate.
  • Add a test case in test/test-parse-warning.cpp based on an existing test case.

@ybl3
Copy link

ybl3 commented Mar 29, 2023

Cool, I claim this for-hire task. I expect payment after I complete this task. I will email the quick-lint-js team if I am assigned this task.
I am doing this for a University of Michigan assignment.

@strager
Copy link
Collaborator Author

strager commented Apr 11, 2023

@ybl3 Do you need any help with this task?

@ybl3
Copy link

ybl3 commented Apr 12, 2023

Is this warning only for || conditions, or for all conditions with string literals? Also, should I consider all string literals, including empty ones ("")? Also, is this warning more geared towards someone that accidentally put quotation marks around a variable name, or just for preventing unnecessary code, or both?

@strager
Copy link
Collaborator Author

strager commented Apr 12, 2023

Is this warning only for || conditions, or for all conditions with string literals?

Good question. I think we should do just || for now.

Also, should I consider all string literals, including empty ones ("")?

Yes. We might want to emit different messages in these different cases.

Also, is this warning more geared towards someone that accidentally put quotation marks around a variable name, or just for preventing unnecessary code, or both?

For case "x"||"y":, the suggestion should be to write case "x": case "y": instead.

For if ("x" || "y"), I don't know what was intended, but we can say that the condition will always be true (similar to E0346).

ybl3 added a commit to ybl3/quick-lint-js that referenced this issue Apr 21, 2023
ybl3 added a commit to ybl3/quick-lint-js that referenced this issue Apr 21, 2023
@ybl3 ybl3 removed their assignment May 2, 2023
@Ripak2005
Copy link

if ("x" === s || "y" === s) {
// Code to execute if s is either "x" or "y"
}

switch (s) {
case "x":
// Code to execute if s is "x"
break;
case "y":
// Code to execute if s is "y"
break;
default:
// Code to execute if s is neither "x" nor "y"
break;
}

@AdityaSripalle
Copy link

The issue lies in the incorrect use of logical operators || within the if statement and the switch case. In C++, logical operators like || are used for combining conditions, but they cannot be directly used to compare values as done in the code snippet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for hire Get paid for working on this task: https://quick-lint-js.com/hiring.html good first issue Good for newcomers and C++ beginners
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants