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

Add no-duplicate-case documentation note about ignoring side effects #15552

Closed
1 task
Dionysusnu opened this issue Jan 30, 2022 · 8 comments · Fixed by #15563
Closed
1 task

Add no-duplicate-case documentation note about ignoring side effects #15552

Dionysusnu opened this issue Jan 30, 2022 · 8 comments · Fixed by #15563
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation rule Relates to ESLint's core rules
Projects

Comments

@Dionysusnu
Copy link

Dionysusnu commented Jan 30, 2022

Environment

Node version: v16.13.1
npm version: 7.13.0
Local ESLint version: v8.7.0
Global ESLint version: N/A
Operating System: Windows 10 Home

What parser are you using?

@typescript-eslint/parser

What did you do?

Configuration
{
	"parser": "@typescript-eslint/parser",
	"parserOptions": {
		"jsx": true,
		"useJSXTextNode": true,
		"ecmaVersion": 2018,
		"sourceType": "module",
		"project": "./tsconfig.json"
	},
	"plugins": ["@typescript-eslint", "prettier"],
	"extends": ["eslint:recommended"],
	"rules": {}
}
let x = 0;
switch (1) {
	case x++: {
		break;
	}
	case x++: {
		break;
	}
}

What did you expect to happen?

Since the code has side effects, the cases being checked are not identical/duplicate, so no error should be generated.

What actually happened?

The code generated a false-positive error on cases that are not duplicate.

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

I realise this is a very niche case, I found this while enabling eslint on unit tests for a compiler, hence the code testing special edge case behaviours.

@Dionysusnu Dionysusnu added bug ESLint is working incorrectly repro:needed labels Jan 30, 2022
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Jan 30, 2022
@mdjermanovic mdjermanovic moved this from Needs Triage to Triaging in Triage Jan 31, 2022
@mdjermanovic
Copy link
Member

Hi @Dionysusnu, thanks for the issue!

I can reproduce this with the default parser as well: online demo.

This is indeed not a duplicate, but I think it's a good case for eslint-disable comments. We could add "When Not To Use It" section in the docs for this rule, mentioning side effects like in no-dupe-else-if rule docs. @eslint/eslint-tsc thoughts?

@Dionysusnu
Copy link
Author

I think it's a good case for eslint-disable comments

While trying that, I noticed that it seemed impossible to do that in a single comment, because of the other rule no-fallthrough. With the code

switch (1) {
	case x++:
	case x++: {
		break;
	}
}

Adding in the eslint-disable comment above the second case, the no-fallthrough rule would start to trigger, probably because it disallows anything but consecutive lines. I didn't find a way to disable both rules in a single comment.

@Dionysusnu
Copy link
Author

I can reproduce this with the default parser as well: online demo.

Have you considered linking this demo in the issue format? I tried looking for something like it but couldn't find it.

@nzakas
Copy link
Member

nzakas commented Feb 1, 2022

The spirit of the rule is to highlight potential copy-paste errors, which this looks like it could be, so I think the current behavior is the expected behavior and disable comments are the right way to deal with exceptions.

@btmills
Copy link
Member

btmills commented Feb 1, 2022

Have you considered linking this demo in the issue format?

@Dionysusnu good idea! #15557 takes a stab at that.

I agree with the rest of the team that side effects feel out of scope for this rule to handle.

@mdjermanovic
Copy link
Member

mdjermanovic commented Feb 1, 2022

While trying that, I noticed that it seemed impossible to do that in a single comment, because of the other rule no-fallthrough. With the code

switch (1) {
	case x++:
	case x++: {
		break;
	}
}

Adding in the eslint-disable comment above the second case, the no-fallthrough rule would start to trigger, probably because it disallows anything but consecutive lines. I didn't find a way to disable both rules in a single comment.

Can you use eslint-disable-line on the line where the duplicate is? That way, no-fallthrough won't report an error because the two cases stay on consecutive lines.

/* eslint no-duplicate-case: error */
/* eslint no-fallthrough: error */

switch (1) {
	case x++:    
	case x++: { // eslint-disable-line no-duplicate-case
		break;
	}
}

Online Demo

@Dionysusnu
Copy link
Author

That works! I thought I'd tried that already, but I guess not properly. Thanks for the help.

@Dionysusnu Dionysusnu changed the title Bug: no-duplicate-case ignores side effects Add no-duplicate-case documentation note about ignoring side effects Feb 1, 2022
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation rule Relates to ESLint's core rules and removed bug ESLint is working incorrectly repro:yes labels Feb 1, 2022
@mdjermanovic mdjermanovic moved this from Triaging to Pull Request Opened in Triage Feb 1, 2022
@mdjermanovic
Copy link
Member

I marked this as an accepted issue to update the docs, and prepared PR #15563

Triage automation moved this from Pull Request Opened to Complete Feb 2, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Aug 2, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

Successfully merging a pull request may close this issue.

4 participants