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

[no-unsafe-member-access] Add exception for optional chaining #2728

Open
3 tasks done
robyoder opened this issue Oct 30, 2020 · 9 comments
Open
3 tasks done

[no-unsafe-member-access] Add exception for optional chaining #2728

robyoder opened this issue Oct 30, 2020 · 9 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@robyoder
Copy link

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

{
  "rules": {
    "@typescript-eslint/no-unsafe-member-access": ["error"]
  }
}
const handleEvent = (event: { detail: any }): void => {
	const uuid: unknown = event.detail?.uuid;
	if (typeof uuid === "string") {
		console.log(uuid);
	}
};
{
	"compilerOptions": {
		"allowSyntheticDefaultImports": true,
		"forceConsistentCasingInFileNames": true,
		"lib": [
			"es5",
			"es2015.promise",
			"es2015.iterable",
			"dom",
			"dom.iterable"
		],
		"incremental": true,
		"module": "esnext",
		"moduleResolution": "node",
		"jsx": "react",
		"noEmitOnError": true,
		"noFallthroughCasesInSwitch": true,
		"noImplicitReturns": true,
		"noUnusedLocals": true,
		"noUnusedParameters": true,
		"resolveJsonModule": true,
		"esModuleInterop": true,
		"sourceMap": false,
		"strict": true,
		"target": "es5"
	}
}

Expected Result

I'd like to not have an error when accessing event.detail?.uuid, either by default or by special rule config, because I'm not seeing what is unsafe about this member access. I've already satisfied the no-unsafe-assignment rule by declaring uuid to be of type unknown, and with optional chaining, it should not be possible to get a runtime error here, no matter what the value of event.detail actually is.

Actual Result

The .uuid access yielded:

error  Unsafe member access .uuid on an any value         @typescript-eslint/no-unsafe-member-access

Versions

package version
@typescript-eslint/eslint-plugin 4.5.0
@typescript-eslint/parser 4.5.0
TypeScript 4.0.3
ESLint 7.11.0
node 14.12.0
@robyoder robyoder added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Oct 30, 2020
@bradzacher
Copy link
Member

I'm not seeing what is unsafe about this member access

The unsafety is that you're operating on an any.
That's all this rule checks - are you doing a member access on an any.

Optional chaning makes it marginally more safe in that you won't get a runtime error for null/undefined - but it's still not safe from a types perspective.

In your specific code sample it's "safe-ish" because you're assigning to a variable explicitly typed as unknown, though you could just as easily not, and as such you'd lose any information about the fact that the value could have been null or undefined.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Oct 30, 2020
@robyoder
Copy link
Author

robyoder commented Nov 2, 2020

Hey, @bradzacher. Thanks for the quick response!

I get what you're saying, but that shouldn't be the responsibility of this rule.

We have other rules (no-unsafe-assigment/call/return) that govern the usage of any values. This rule (no-unsafe-member-access) should only govern unsafe access. My example shows a safe access that yields an unsafe value. That unsafe value is then assigned to a variable, which no-unsafe-assignment ensures is typed safely (unknown).

It sounds like you're concerned that I could do something with that any value that is unsafe that wouldn't be protected by no-unsafe-assigment (et al). That is true. But take the following example:

const getEvent = (): { detail: { uuid: any } } => ({
    detail: { uuid: null },
});

const logString = (t: string): void => {
    console.log(t);
}

const handleEvent = (): void => {
    logString(getEvent().detail.uuid);
}

Now, ignore the getEvent function return type violating no-explicit-any. Let's say that function comes from third-party code. My config has all of the following enabled:

  • no-unsafe-member-access
  • no-unsafe-assigment
  • no-unsafe-return
  • no-unsafe-call

But no linter flags are raised inside the handleEvent function, even though I have an any value that's being unsafely passed into a function that expects a string value.

My question is how is that different from my original example? Or even simpler, how is { thing: any }.thing safer than any?.thing? And my argument is that it isn't. I'm not saying this is great; rather that it's not the responsibility of no-unsafe-member-access to prevent you from arriving at an unsafe (any) value, only to prevent you from performing an unsafe access. The example above illustrates a need for another rule that prevents unsafe use of any values beyond assignments, returns, and calls, and whatever rule that is would prevent me from doing anything unsafe with my any value from the original example.

I hope that made sense. Curious to hear your thoughts.

@robyoder
Copy link
Author

robyoder commented Nov 2, 2020

The example above illustrates a need for another rule that prevents unsafe use of any values beyond assignments, returns, and calls, and whatever rule that is would prevent me from doing anything unsafe with my any value from the original example.

This is worth its own issue being filed, of course, but it makes sense to me to resolve this discussion first.

@bradzacher
Copy link
Member

bradzacher commented Nov 2, 2020

Your example is just not handled by any rule right now.
It's on the todo list, but I haven't had the time to implement it, and no contributor has been driven enough to implement it.
See #791 (comment)


In the ESLint world, you can't assume anything about the user's config. You can't assume that a user has rule X turned on because they're using rule Y - each rule is entirely isolated from one-another. Which is why this rule just assumes you're using it in isolation.
In an ideal world, these rules would be just a single rule. But there are various reasons (that I won't get into) why these are separate rules.


If you want to add a flag (default false) which allows the rule to ignore optional chains on anys (i.e. so it can better coexist with the other rules) happy to accept a PR.

@bradzacher bradzacher added enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed awaiting response Issues waiting for a reply from the OP or another party labels Nov 2, 2020
@robyoder
Copy link
Author

robyoder commented Nov 6, 2020

In the ESLint world, you can't assume anything about the user's config. You can't assume that a user has rule X turned on because they're using rule Y - each rule is entirely isolated from one-another. Which is why this rule just assumes you're using it in isolation.

Sure. But my main point is that { thing: any }.thing is no safer than any?.thing, so if the latter is something this rule addresses, so should the former be. And conversely if the former is not a concern of this rule, neither should the latter be. :)

Anyway, I'll make a note to try to implement this myself. Thanks for the feedback and the link to your comment about no-unsafe-argument. 👍

@JasCodes

This comment was marked as off-topic.

@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 25, 2021
@geoquant

This comment was marked as off-topic.

@JasCodes

This comment was marked as off-topic.

@JoshuaKGoldberg

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

5 participants