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

Using no-else-return makes code broken #156

Closed
warmrobot opened this issue Dec 29, 2021 · 1 comment · Fixed by #160
Closed

Using no-else-return makes code broken #156

warmrobot opened this issue Dec 29, 2021 · 1 comment · Fixed by #160

Comments

@warmrobot
Copy link

warmrobot commented Dec 29, 2021

Hello, everyone!
I was very surprised to found this dangerous case with a single eslint rule. I can reproduce it.

Initial test.svelte:

<script>
  function getSvgPathDProperty(data) {
    return data.reduce((accumulator, point, index, data) => {
      if (index === 0) {
        return `M ${point.x},${point.y}`;
      } else {
        return `${accumulator} ${bezierCommand(point, index, data)}`;
      }
    }, "");
  }
</script>

After eslint --fix

<script>
  function getSvgPathDProperty(data) {
    return data.reduce((accumulator, point, index, data) => {
      if (index === 0) {
        return `M ${point.x},${point.y}`;
      } 
      return `${accumulator} ${bezierCommand(point, index, data)}`;
       }    // <-- curly brace, and it seems to have one space before, so strange o_O
    }, "");
  }
</script>

And this code no longer valid, because it leaves extra curly brace after second return.
Congratulations for those who using --fix on precomit. )

.eslintrc.js with just one rule:

module.exports = {
	parser: '@typescript-eslint/parser', // add the TypeScript parser
	plugins: [
	  'svelte3',
	  '@typescript-eslint' // add the TypeScript plugin
	],
	overrides: [ // this stays the same
	  {
		files: ['*.svelte'],
		processor: 'svelte3/svelte3'
	  }
	],
	rules: {
		'no-else-return': 1
	},
	settings: {
	  'svelte3/typescript': () => require('typescript'), // pass the TypeScript package to the Svelte plugin
	}
  };

package.json:

  "scripts": {
    "eslint": "eslint test.svelte --fix"
  },
  "dependencies": {
    "@typescript-eslint/eslint-plugin": "^5.8.1",
    "@typescript-eslint/parser": "^5.8.1",
    "eslint": "^7.32.0",
    "eslint-plugin-svelte3": "^3.2.1",
    "svelte": "^3.44.3",
    "typescript": "^4.5.4"
  }

Any ideas why it's broken only in *.svelte files, not *.js?
By the way, I think no-lonely-if also lead to problems in some cases. I wil fill another issue with that.

@dummdidumm
Copy link
Member

Likely same reason as #110

dummdidumm pushed a commit to dummdidumm/eslint-plugin-svelte3 that referenced this issue Jan 5, 2022
Fixes don't need to start at the same line where the error message is shown, and they don't need to be at the same line as the start of the error message. Therefore the previous handling of re-adding dedents was flawed. Instead, map the range to positions (line/character) and use them to find the correct dedent offsets.
Additionally add indentation after each newline a fix has because the fix doesn't know about the additional indentation.

To properly test fixes, the test setup was enhanced. If there's a Fixed.svelte, that one will be compared against a fixed version of Input.svelte.

Fixes sveltejs#110
Fixes sveltejs#156
Fixes sveltejs#157
dummdidumm added a commit that referenced this issue Jan 5, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Fixes don't need to start at the same line where the error message is shown, and they don't need to be at the same line as the start of the error message. Therefore the previous handling of re-adding dedents was flawed. Instead, map the range to positions (line/character) and use them to find the correct dedent offsets.
Additionally add indentation after each newline a fix has because the fix doesn't know about the additional indentation.

To properly test fixes, the test setup was enhanced. If there's a Fixed.svelte, that one will be compared against a fixed version of Input.svelte.

Fixes #110
Fixes #156
Fixes #157
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 a pull request may close this issue.

2 participants