feat: Add `svelte/valid-context-access` rule #480

baseballyama · 2023-05-14T11:05:12Z

close: #448

changeset-bot · 2023-05-14T11:05:15Z

🦋 Changeset detected

Latest commit: ffc0bce

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
eslint-plugin-svelte	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ota-meshi

Thank you for this PR!
I haven't checked all yet, but I have some comments.

ota-meshi · 2023-05-28T08:44:17Z

src/rules/valid-context-access.ts

+      currentNode: TSESTree.CallExpression,
+    ) {
+      let { parent } = currentNode
+      if (parent?.type !== "ExpressionStatement") {


I suspect an if statement like the following will not work:

if (hasContext("answer")) { }

Could you add this test case?

I added a basic test for if statement. I think it works fine.
Is this same understanding with you?

a86eac8

Sorry. I should have explained more.
I would like to add a test for if statements are used at the top level.

<script> import { hasContext } from "svelte" if (hasContext("answer")) { console.log("The answer exist") } </script>

Oh OK! I will add a test and fix logic.

ota-meshi · 2023-05-28T08:46:37Z

src/rules/valid-context-access.ts

+    },
+    type: "problem",
+  },
+  create(context) {


It probably doesn't work well other then *.svelte files, so I think the following guard is needed.

Suggested change

create(context) {

create(context) {

if (!context.parserServices.isSvelte) {

return {}

}

I need to add more test but even JS file can use onMount so I think we need to check JS files also.

import { setContext, onMount } from "svelte" const something = () => { setContext("answer", 42) } onMount(() => something())

But at the same time, I found this pattern.
It should be work but now ESLint error occurs.
So I need to handle this.

import { setContext } from "svelte" const something = () => { setContext("answer", 42) } const wrapper = (fn) => { fn() } wrapper(() => something())

In addition, I forgot to handle async/await.😇

const something = async () => { await Promise.resolve() setContext("answer", 42) } something()

I added this logic and I wrote this limitation on the docs.

baseballyama · 2023-05-29T16:24:01Z

I realized that it’s impossible to check async/await things perfectly.
For example below case, it will be an error but I think it isn't easy to detect it by ESLint.

So my opinion is that we will mention this limitation on the docs and we don't check async/await things.
Or we don't implement this rule.
There may not be much value in the rule that includes this false negative.
What do you think @ota-meshi ?

<script>
import { setContext } from "svelte"
import someAsyncProcess from './outside'

someAsyncProcess(() => {
  setContext("answer", 42)
});

// outside.js
export async function someAsyncProcess(fn) {
  await Promise.resolve();
  fn();
}

ota-meshi · 2023-05-30T05:25:57Z

Hmm. You're right, there are false negatives, but I don't think users will use setContext that way, so I don't think it matters much if it's not reported.

I think code like the following is a common mistake, so I think there's value in having it reported by a rule.

const something = async () => {
  await Promise.resolve()
  setContext("answer", 42)
}

eslint-plugin-vue has some similar checking rules.
https://eslint.vuejs.org/rules/no-expose-after-await.html
https://eslint.vuejs.org/rules/no-lifecycle-after-await.html
https://eslint.vuejs.org/rules/no-watch-after-await.html
https://eslint.vuejs.org/rules/no-restricted-call-after-await.html

baseballyama · 2023-06-03T13:10:41Z

src/rules/infinite-reactive-loop.ts

+import { extractSvelteLifeCycleReferences } from "./reference-helpers/svelte-lifecycle"
+import { extractTaskReferences } from "./reference-helpers/microtask"


I moved few functions to utils dir because I want to use these in valid-context-access rule.

baseballyama · 2023-06-03T13:11:07Z

src/rules/infinite-reactive-loop.ts

+    const tickCallExpressions = Array.from(
+      extractSvelteLifeCycleReferences(context, ["tick"]),
+    )
+    const taskReferences = Array.from(extractTaskReferences(context))
+    const reactiveVariableReferences = getReactiveVariableReferences(context)


Just I moved there lines for performance improvement.

baseballyama · 2023-06-03T13:11:40Z

src/rules/valid-context-access.ts

+    },
+    type: "problem",
+  },
+  create(context) {


I added this logic and I wrote this limitation on the docs.

ota-meshi · 2023-06-10T06:47:31Z

src/rules/valid-context-access.ts

+    const awaitExpressions: {
+      belongingFunction:
+        | TSESTree.FunctionDeclaration
+        | TSESTree.VariableDeclaration


I don't think the belongingFunction should be assigned a VariableDeclaration.
Should we have checked FunctionExpression instead?

ota-meshi · 2023-06-10T06:51:51Z

src/rules/valid-context-access.ts

+    function isAfterAwait(node: TSESTree.CallExpression) {
+      for (const awaitExpression of awaitExpressions) {
+        const { belongingFunction, node: awaitNode } = awaitExpression
+        if (isInsideOf(node, belongingFunction)) {


There seems to be false positives in complex cases such as:

<script> import { getContext } from "svelte" async function fn() { async function foo() { await p } getContext("a") await foo() } fn() </script>

I don't think we can check the correct function scope by just checking the range.

ota-meshi · 2023-06-10T07:09:50Z

src/rules/valid-context-access.ts

+        parent = parent.parent
+        if (
+          parent?.type === "VariableDeclaration" ||
+          parent?.type === "FunctionDeclaration"
+        ) {
+          const references =
+            parent.type === "VariableDeclaration"
+              ? getReferences(parent.declarations[0].id)
+              : parent.id
+              ? getReferences(parent.id)
+              : []
+
+          for (const reference of references) {
+            if (reference.identifier?.parent?.type === "CallExpression") {
+              if (
+                !visitedCallExpressions.includes(reference.identifier.parent)
+              ) {
+                visitedCallExpressions.push(reference.identifier.parent)
+                doLint(
+                  visitedCallExpressions,
+                  contextCallExpression,
+                  reference.identifier?.parent,
+                )
+              }
+            }
+          }
+        } else if (parent?.type === "ExpressionStatement") {
+          if (parent.expression.type !== "CallExpression") {
+            report(contextCallExpression)
+          } else if (lifeCycleReferences.includes(parent.expression)) {
+            report(contextCallExpression)
+          }
+        }


I think it's safer to check the VariableDeclarator. Also, I think we should check if the right hand side operand is a function expression.
Otherwise, the following cases will result in false positives.

<script> import { getContext, onMount } from "svelte" const foo = getContext("foo") onMount(() => { foo() }) </script>

Suggested change

parent = parent.parent

if (

parent?.type === "VariableDeclaration" ||

parent?.type === "FunctionDeclaration"

) {

const references =

parent.type === "VariableDeclaration"

? getReferences(parent.declarations[0].id)

: parent.id

? getReferences(parent.id)

: []

for (const reference of references) {

if (reference.identifier?.parent?.type === "CallExpression") {

if (

!visitedCallExpressions.includes(reference.identifier.parent)

) {

visitedCallExpressions.push(reference.identifier.parent)

doLint(

visitedCallExpressions,

contextCallExpression,

reference.identifier?.parent,

)

}

}

}

} else if (parent?.type === "ExpressionStatement") {

if (parent.expression.type !== "CallExpression") {

report(contextCallExpression)

} else if (lifeCycleReferences.includes(parent.expression)) {

report(contextCallExpression)

}

}

if (

(parent?.type === "VariableDeclarator" &&

parent.init &&

(parent.init.type === "FunctionExpression" ||

parent.init.type === "ArrowFunctionExpression") &&

isInsideOf(parent.init, currentNode)) ||

parent?.type === "FunctionDeclaration"

) {

const references = parent.id ? getReferences(parent.id) : []

for (const reference of references) {

if (reference.identifier?.parent?.type === "CallExpression") {

if (

!visitedCallExpressions.includes(reference.identifier.parent)

) {

visitedCallExpressions.push(reference.identifier.parent)

doLint(

visitedCallExpressions,

contextCallExpression,

reference.identifier?.parent,

)

}

}

}

} else if (parent?.type === "ExpressionStatement") {

if (parent.expression.type !== "CallExpression") {

report(contextCallExpression)

} else if (lifeCycleReferences.includes(parent.expression)) {

report(contextCallExpression)

}

}

parent = parent.parent

baseballyama force-pushed the feat/448 branch 2 times, most recently from 49ff328 to 43d6041 Compare May 14, 2023 11:08

baseballyama force-pushed the feat/448 branch from 43d6041 to 917587f Compare May 27, 2023 08:56

baseballyama marked this pull request as ready for review May 28, 2023 06:26

baseballyama requested a review from ota-meshi May 28, 2023 06:26

ota-meshi requested changes May 28, 2023

View reviewed changes

baseballyama added 4 commits May 29, 2023 14:04

implement

7e51ff3

add changeset

2c57127

add test

05f8917

add if statement test

a86eac8

baseballyama force-pushed the feat/448 branch from c1c2c7b to a86eac8 Compare May 29, 2023 05:04

fix if statement and support context module

72dffcc

baseballyama force-pushed the feat/448 branch from 6b8edb4 to 72dffcc Compare June 3, 2023 05:09

baseballyama added 8 commits June 3, 2023 15:53

fix function argument

d7d0449

support await

9e01170

Merge remote-tracking branch 'upstream/main' into feat/448

bcdb3a9

refactor

972e900

refactor

0f50d57

support setTimeout and others

736e5a1

support then / catch

6ff0994

add tests and fix bug

ffc0bce

baseballyama commented Jun 3, 2023

View reviewed changes

baseballyama requested a review from ota-meshi June 3, 2023 13:12

ota-meshi requested changes Jun 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `svelte/valid-context-access` rule #480

feat: Add `svelte/valid-context-access` rule #480

baseballyama commented May 14, 2023

changeset-bot bot commented May 14, 2023 •

edited

ota-meshi left a comment

ota-meshi May 28, 2023

baseballyama May 29, 2023

ota-meshi May 29, 2023

baseballyama May 29, 2023 •

edited

ota-meshi May 28, 2023

baseballyama May 29, 2023

baseballyama Jun 3, 2023

baseballyama commented May 29, 2023

ota-meshi commented May 30, 2023 •

edited

baseballyama Jun 3, 2023

baseballyama Jun 3, 2023

baseballyama Jun 3, 2023

ota-meshi Jun 10, 2023

ota-meshi Jun 10, 2023

ota-meshi Jun 10, 2023 •

edited

		import { extractSvelteLifeCycleReferences } from "./reference-helpers/svelte-lifecycle"
		import { extractTaskReferences } from "./reference-helpers/microtask"

feat: Add svelte/valid-context-access rule #480

Are you sure you want to change the base?

feat: Add svelte/valid-context-access rule #480

Conversation

baseballyama commented May 14, 2023

changeset-bot bot commented May 14, 2023 • edited

🦋 Changeset detected

ota-meshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baseballyama May 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baseballyama commented May 29, 2023

ota-meshi commented May 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ota-meshi Jun 10, 2023 • edited

Choose a reason for hiding this comment

feat: Add `svelte/valid-context-access` rule #480

feat: Add `svelte/valid-context-access` rule #480

changeset-bot bot commented May 14, 2023 •

edited

baseballyama May 29, 2023 •

edited

ota-meshi commented May 30, 2023 •

edited

ota-meshi Jun 10, 2023 •

edited