`[READ]` or `[CALL]`?

[aladdin-add]

          I see `enableCompileCache`, `getCompileCacheDir` and other methods use `READ`, but since they're function to be called they should all use `CALL` I presume?

Hence why I used CALL instead of READ here as well

Originally posted by @MichaelDeBoey in #358 (comment)

[scagood]

Just gonna repost my comment here:
#358 (comment)

I just mark everything as READ, as we're checking if it's there or not. 🤔

I am happy to change that you both think CALL is more reasonable.

module.enableCompileCache; // Not matched
module.enableCompileCache(); // Matched

// I have not tested, I assume this does not match
module.enableCompileCache.call(module); // Not matched ?

[MichaelDeBoey]

@aladdin-add's response (#358 (comment)):

module.enableCompileCache.call(module); // Not matched ?

yes, I see, and confirmed it's not matched.

just opened #359 for discussion. For now, for consistency, let's just use [READ] - we can change all these to [CALL] once reached consensus. 😄

[MichaelDeBoey]

This is the way I understood the 3 symbols:

• READ if it's a variable

• CALL if it's a function

• CONSTRUCT if it's a class that can be constructed (with new)
  On this part I'm still not 100% sure as the global CustomerEvent doesn't have 'class' next to it, but the one from the events module has 🤔

[scagood]

I am currently in favour of using READ everywhere as there are too many ways to easily avoid CALL and CONSTRUCT 🤔

Examples that are not CONSTRUCT but probably should be marked:

if (event instanceof CustomEvent) {}
class MyEvent extends CustomEvent {}

Examples that are not CALL but probably should be marked:

module.enableCompileCache.call(module); // or `bind`/`apply`
process.nextTick(module.enableCompileCache);

---

That being said, if people are going to go to that length, maybe we should let them?

[MichaelDeBoey]

I would argue that we would need to change the code to enable

if (event instanceof CustomEvent) {}
class MyEvent extends CustomEvent {}

if we set it as CONSTRUCT

And that we would also enable

module.enableCompileCache.call(module); // or `bind`/`apply`

if we set CALL

process.nextTick(module.enableCompileCache) would be nice if it's enabled as well, but I guess that won't be easy to implement on our side 🤔

[scagood]

This is feeling like an https://github.com/eslint-community/eslint-utils discussion to me 🤔

[MichaelDeBoey]

@scagood Since these types are coming from @eslint-community/eslint-utils I kinda agree.
On the other hand: it would be nice to know what that lib needs to change in order for us to be able to move forward on this lib as well

CC/ @eslint-community/eslint-utils Tagging the eslint-utils team to get their input as well

[MichaelDeBoey]

After some further investigation I came across the following explanation from eslint-utils' tracker.iterateGlobalReferences docs, confirming my thought process I mentioned above:

The reference type. If this is ReferenceTracker.READ then it read the variable (or property). If this is ReferenceTracker.CALL then it called the variable (or property). If this is ReferenceTracker.CONSTRUCT then it called the variable (or property) with the new operator.

But I guess it will need some more research to know whether we (eslint-plugin-node) or @eslint-community/eslint-utils is not handling the above mentioned examples as expected before we would know where to fix them accordingly (at least for me as I don't have enough in depth knowledge of both libs) 🤔

[fisker]

I always wonder what should be used to get delete foo and delete globalThis.foo. It's confusing.

[MichaelDeBoey]

@fisker I guess the answer would be 'no', but would there be an easy way to test out how that currently works on the @eslint-community/eslint-utils side?

[fisker]

If I remember correctly, [READ] can get them, I used in eslint-plugin-unicorn.