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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(eslint-plugin): [prefer-function-type] handle `this` return #2437

Merged
merged 11 commits into from Sep 14, 2020

Conversation

@tadhgmister
Copy link
Contributor

@tadhgmister tadhgmister commented Aug 29, 2020

Fixes #1756

This isn't quite ready yet, it is missing update to docs and a few cases described below. Just need some feedback for how to handle some edge cases.

When this is used in a call signature of an interface the auto-fix will replace the occurrences of this with the name of the interface so the suggested fix is still valid. However: If the interface defines type parameters and uses this then the necessary replacement becomes much more tricky, for example:

interface Foo<A extends string, B = number> {
  (a: A, b: B): this;
}
// correct replacement would be:
type Foo<A extends string, B = number> = (a: A, b: B) => Foo<A,B>;

The current handling for generics can get away with just coping the generics declaration without knowing how to reconstruct Foo<A,B> as the valid reference to it's own interface name, so handling this would take some additional effort that I personally don't think is really worth it. (Also I don't trust myself to produce code to do it that is bug free)

Right now my implementation never reports on an interface with both type parameters and refers to this but I don't think that is the best course of action, I'd appreciate some advice on what the best course of action would be in this case.

The second part is that I think the intent was to offer a suggestion instead of auto fix when this is used since technically the suggested fix isn't strictly equivalent. I'd like some advice on whether the message should be the same or if there is a different message that should show.

Thank you! 馃槃

@typescript-eslint
Copy link

@typescript-eslint typescript-eslint bot commented Aug 29, 2020

Thanks for the PR, @tadhgmister!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


馃檹 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@codecov
Copy link

@codecov codecov bot commented Aug 29, 2020

Codecov Report

Merging #2437 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2437   +/-   ##
=======================================
  Coverage   92.81%   92.82%           
=======================================
  Files         290      290           
  Lines        9453     9466   +13     
  Branches     2647     2650    +3     
=======================================
+ Hits         8774     8787   +13     
  Misses        322      322           
  Partials      357      357           
Flag Coverage 螖
#unittest 92.82% <酶> (+<0.01%) 猬嗭笍

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage 螖
...es/eslint-plugin/src/rules/prefer-function-type.ts 96.29% <酶> (+1.17%) 猬嗭笍
@bradzacher bradzacher added the bug label Aug 29, 2020
@bradzacher bradzacher marked this pull request as draft Aug 29, 2020
@bradzacher
Copy link
Member

@bradzacher bradzacher commented Aug 29, 2020

See prior art: #1783

@tadhgmister
Copy link
Contributor Author

@tadhgmister tadhgmister commented Aug 29, 2020

second question, the coverage went down slightly because tsThisTypes?.push and tsThisTypes ?? undefined is only hit in cases where tsThisTypes is an actual list so the optional part is never checked, I assume that is fine? If there is something different I should do that would also be welcome feedback.

@bradzacher
Copy link
Member

@bradzacher bradzacher commented Aug 29, 2020

Suggestion fixers are a good alternative for risky fixers, but here I would probably err on the side of "fix it properly or not at all". We don't want to provide a suggestion that's slightly wrong and then break all of their types.

Note that we don't have to provide a fixer for all cases.
If it's too complicated to produce valid code automatically then it's perfectly fine for us to just report without a fixer.
The user can always manually fix up their code. ESP in this sort of case where it's already pretty advanced TS type - the user should be well-versed enough to be able to manually convert it.


codecov doesn't handle optional chaining well. The test coverage is just a guide, if you can cover all of the branches, fine, if there are cases that can't be hit, then feel free to ignore. I will make a judgement when merging to decide if the tests are good enough.

@bradzacher bradzacher changed the title fix(eslint-plugin): handle 'this' in prefer-function-type fix(eslint-plugin): [prefer-function-type] handle `this` return Aug 29, 2020
@tadhgmister
Copy link
Contributor Author

@tadhgmister tadhgmister commented Aug 30, 2020

I have thought about this more thoroughly and have come to the conclusion that using this doesn't make any sense here. It will refer to the function type which doesn't seem like a useful construct, when people do return this in javascript to do method chaining it's rarely

I think it'd be likely that if people wanted to write a mixin-style method (independent of class but uses this) they would want to use a generic this parameter instead of referring to the this of the function itself.

function logArg<Self>(this: Self, arg: string): Self {
  console.log(`logging ${arg} from ${this.toString()}`);
  return this;
}
const thing = {logArg, data: "hello"};
// the function returns `thing` in this case, not the `logArg` function:
const data = thing.logArg("logged to console").data;

this is a useful construct, and in this case using type MemberMixin = <Self>(this: Self, ...) => Self would be the correct way to do it, I can absolutely see someone accidentally ending up with just using the this type and be surprised when it refers to the function.

So I think if there is an interface that defines only a call signature and uses this they should get an error message like:

this refers to the function type {{ interfaceName }}, did you intend to use a generic this parameter like <Self>(this: Self, ...) => Self instead?

(Offer no suggestion or fix in this case, it represents a semantic change that is application specific, I imagine every application would have some constraint on the Self type for the method to be useful.)

This will probably help people move towards what they almost certainly intended in the first place, and if they do replace all occurrences of this then the current implementation of the auto fix is absolutely applicable.

Does this seem reasonable?


I'm also not sure how to approach this in the docs, is an update needed to mention this or does it fall under "obscure enough edge case we don't have to publicize it"?

Tadhg McDonald-Jensen and others added 3 commits Aug 30, 2020
Tadhg McDonald-Jensen
@tadhgmister tadhgmister marked this pull request as ready for review Sep 3, 2020
@tadhgmister tadhgmister marked this pull request as draft Sep 3, 2020
@tadhgmister tadhgmister marked this pull request as ready for review Sep 3, 2020
@tadhgmister tadhgmister marked this pull request as draft Sep 3, 2020
Tadhg McDonald-Jensen added 2 commits Sep 3, 2020
excluding nested type literals wasn't as strait forward as I thought
excluding the code to do so because it's not a very common use case so
I'm not worrying about it.
will now not say "this refers to Foo" on uses of `this` that are invalid
@tadhgmister tadhgmister marked this pull request as ready for review Sep 3, 2020
@tadhgmister
Copy link
Contributor Author

@tadhgmister tadhgmister commented Sep 3, 2020

@bradzacher I think it's ready now, still not really sure if or how to mention this in the docs, if you think something should be added please tell me and I'll come up with something, otherwise this should be good to go.

Thanks.

Copy link
Member

@bradzacher bradzacher left a comment

logic mostly LGTM.
One bug that needs to be fixed.

thanks for your work.


Responses to your questions:

So I think if there is an interface that defines only a call signature and uses this they should get an error message like:
.....
Does this seem reasonable?

Yup, that seems perfectly reasonable to me. I'm all for having lint rules clearly educate users about how TS actually works (see ban-types where I added some education that people constantly butt up against).

I'm also not sure how to approach this in the docs, is an update needed to mention this or does it fall under "obscure enough edge case we don't have to publicize it"?

I would probably just add it as a examples and add some comments to it.
I don't see a problem of clearly educating people ahead of time as well. I.e.


Examples of incorrect code for this rule:

<snip>

interface Foo {
  (arg: number): this | undefined;
}

interface Foo {
  (arg: this): void;
}

Examples of correct code for this rule:

<snip>

type Foo<TSelf> = (this: TSelf, arg: number) => TSelf | undefined;

type Foo<TSelf> = (this: TSelf, arg: TSelf) => void
Tadhg McDonald-Jensen added 2 commits Sep 11, 2020
@tadhgmister tadhgmister requested a review from bradzacher Sep 11, 2020
Copy link
Member

@bradzacher bradzacher left a comment

LGTM - thanks for your contribution!

@bradzacher bradzacher merged commit 7c6fcee into typescript-eslint:master Sep 14, 2020
10 checks passed
10 checks passed
Typecheck
Details
Unit tests Unit tests
Details
Code style and lint
Details
Run integration tests on primary Node.js version
Details
Run unit tests on other Node.js versions (10.x)
Details
Run unit tests on other Node.js versions (14.x)
Details
Publish the latest code as a canary version
Details
Semantic Pull Request ready to be squashed
Details
codecov/patch Codecov Report
Details
codecov/project Codecov Report
Details
phaux added a commit to phaux/typescript-eslint that referenced this pull request Sep 28, 2020
鈥cript-eslint#2437)

Co-authored-by: Tadhg McDonald-Jensen <tadhg.mcdonaldjensen@carleton.ca>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can鈥檛 perform that action at this time.