Skip to content

Update no-explicit-any.md #7355

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

Closed
wants to merge 6 commits into from

Conversation

ericelliott
Copy link

@ericelliott ericelliott commented Jul 28, 2023

Fix potentially misleading statement about explicit any - which is sometimes required for functional code.

PR Checklist

Notes:

This checklist discourages people from contributing PRs by trying to force us to wait around for "accepting prs". If you don't want to accept a PR, just close it. Bonus: It gives you one spot to decide whether to accept a PR, instead of 2.

Overview

Provide an example of when explicit any may be needed, and how to disable the rule on an as-needed basis.

Fix potentially misleading statement about explicit `any` - which is sometimes required for functional code. Provide an example of when explicit `any` may be needed, and how to disable the rule on an as-needed basis.
@typescript-eslint
Copy link
Contributor

Thanks for the PR, @ericelliott!

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.

@netlify
Copy link

netlify bot commented Jul 28, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 4907378
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/64c5d8a2ff3b4c0008ad2b71
😎 Deploy Preview https://deploy-preview-7355--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@nx-cloud
Copy link

nx-cloud bot commented Jul 28, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4907378. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 35 targets

Sent with 💌 from NxCloud.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know it's going to be a fun time when someone files an issue mentioning Haskell and the PR uses a phrase like "fully co-expressive". 😄

A good start, but let's workshop the direction & phrasing. Thanks!

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jul 28, 2023
@@ -6,8 +6,8 @@ description: 'Disallow the `any` type.'
>
> See **https://typescript-eslint.io/rules/no-explicit-any** for documentation.

The `any` type in TypeScript is a dangerous "escape hatch" from the type system.
Using `any` disables many type checking rules and is generally best used only as a last resort or when prototyping code.
The `any` type in TypeScript is a potentially dangerous "escape hatch" from the type system.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Praise] +1, heh - this existing opening sentence is a bit strong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What isn't dangerous about any?
It literally opts you out of every single check that TS might provide - there's nothing that isn't dangerous about that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-thinking after reading your #7354 (comment): yeah you're right. I was coming from the perspective that it can be necessary and, when handled carefully, relatively safe. But as you said:

Your example is only safe purely because you've explicitly annotated the variables to "hide" the any. Essentially you've allowed the unsafety and covered it up quickly.

(and the rest of that paragraph)

Marking as resolved, rescinding my +1 😄

Copy link
Author

@ericelliott ericelliott Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradzacher I could go deep into the empirical data behind type safety but this isn't the right forum for a debate. I think it's enough to say that there are some useful things that you just can't do in TypeScript without any - foundational things like proper function composition (see the code example) which can eliminate a huge amount of code in your application and with it, eliminate all the bugs that may have cropped up in the code that got eliminated. In other words, there are cases where using any is a net win for safety, by a huge margin.

In this particular case, it's more dangerous to think of any as a dangerous error than it is to allow it because of the danger it saves us from.

Copy link
Member

@bradzacher bradzacher Jul 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree that any is safe anywhere.
I get that there are cases that you can't type with TS and are stuck using an any or ts ignores - but that doesn't make the usecases safe.

As mentioned in the issue - the only reason your example code is "safe" is because you've hidden the any with an explicit annotation. Had you forgotten the explicit annotation then your code would be in an unsafe state. This approach is simply not scalable though - in small teams it can be sufficient to find and catch in code reviews and thus "make things safe" - but at scale you can't rely on such tribal knowledge and the pattern quickly devolves into an unsafe mess.

Using any is unsafe. Not potentially - it just is.

Copy link
Author

@ericelliott ericelliott Jul 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That dogmatic opinion is not supported by evidence. If you have evidence to support your statement, please supply it. Otherwise, I refer you to the provably true statement: "[function composition] can eliminate a huge amount of code in your application and with it, eliminate all the bugs that may have cropped up in the code that got eliminated." In other words, whether or not static types actually have any causal relationship with bug density (a statement which lacks empirical support), we can say with objective certainty, in the specific case of function composition, any can be empirically shown to reduce the likelihood of bugs by reducing the surface area of code - which, regardless of bug density, will reduce the total amount of code that bugs could appear in, and hence, reduce the total number of bugs over-all.

Empirical Evidence:

  • This article explains how function composition reduces the total amount of code needed, with specific examples of both React Higher Order Component composition and server side request handling middleware.
  • There is no known causal empirical relationship between language choice (including type safety) and defect rates http://janvitek.org/pubs/toplas19.pdf

This paper specifically looking at TypeScript vs JavaScript found:

  • TypeScript apps were actually more bug prone with longer bug resolution times compared to JavaScript. This goes against the expectation that static typing improves bug rates.
  • TS projects needed on average more than an additional day to fix bugs (31.86 vs. 33.04 days) (no causal relationship proven either way).
  • Within TypeScript projects, reduced usage of the "any" type (which ignores type safety) correlated with better code quality and faster bug resolution times. But it did not correlate with fewer bugs. This supports using any as a last resort, but does not support the statement that any is "dangerous".

The paper concludes that while TypeScript has benefits like code quality, it does not directly reduce bugs versus JavaScript in this sample. The perceived advantage of type safety for bug prevention was not observed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're thinking of pointing to the widely cited "To Type or Not to Type" whose conclusions were specifically rejected by the last article I mentioned above, you should know the original "To Type or Not to Type" article is the primary source I used to show a very small relationship between static types and bug density in The TypeScript Tax because the study showed that "specification errors caused about 78% of the publicly classified bugs studied on GitHub". This implies that type safety cannot be the primary safety net that you use to prevent bug defects, and after applying other safety measures (design review, tdd, code review) the number of remaining bugs is very small, so even factoring for a further 20% bug reduction from TypeScript (which the paper did not achieve, even knowing the root cause of the bugs studied and specifically trying to solve for them using TypeScript typings), the difference in total bugs found is quite small, relatively speaking:

typescript-bug-reduction-after-quality-measures

Copy link
Author

@ericelliott ericelliott Jul 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last point: The back-and-forth in THIS PR about attempts to improve type safety using features like generics to try to make composition type safe reveals examples where the attempt to eliminate any in the type definition actually introduced bugs, breaking the type checking for compose and pipe by making the types more restrictive than they should have been, or leading to incorrect typings for the functions.

This supports my assertion that sometimes, just using any is safer than trying to remove it from your code.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Jul 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're talking past each other. The following points are ones we seem to agree on:

  • Type correctness does not guarantee program correctness (intentionally quoting your article): agreed ✅
  • Functional programming paradigms improve program quality: agreed ✅
  • any is sometimes necessary in TypeScript to fully represent type signatures not representable in its type system: agreed ✅
  • any should not be used when a more appropriate type representation is available: agreed ✅ with the caveat that we might not agree on what is more appropriate when
  • Many cases of any outside of limited necessary uses reduce TypeScript's usefulness: agreed ✅ because by definition, features such as go-to-definition and property name checking are turned off on anys

The following two opinions are considered canon in this repository. It would be a waste of time to debate them here, as they're generally-accepted TypeScript best practices and discussing them would quickly devolve into huge discussions that likely won't change anybody's mind:

  • any is dangerous: even if it's necessary, it's very easy to get wrong
    • Something can be dangerous and necessary. Using as type assertions is sometimes necessary too, but they're also -to a lesser extent- dangerous
  • Type correctness improves program quality
    • Caveat: it can't be the primary safety net (of course!)
    • You might only agree with us that it improves developer capabilities, but I don't think it's necessary for our discussion to debate whether it reduces bugs

The main contentious point seems to be the intent and wording around whether to use any as a backup / last resort. Yes, there are cases where using any is better than an incorrect type description. If (or, I hope, when) we do take in a PR mentioning that, it'll probably end up being 2-3 sentences at most. We don't need to pull in all of programming and paradigm type theory to discuss those 2-3 sentences.

I'll post a root comment on this PR. edit: #7355 (comment)

Clarify the need for `any` by making use of `compose` and `pipe` to compose functions with mismatched type signatures.
@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #7355 (4907378) into main (6addee9) will increase coverage by 0.00%.
Report is 9 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7355   +/-   ##
=======================================
  Coverage   87.41%   87.41%           
=======================================
  Files         381      381           
  Lines       13313    13316    +3     
  Branches     3934     3936    +2     
=======================================
+ Hits        11637    11640    +3     
  Misses       1298     1298           
  Partials      378      378           
Flag Coverage Δ
unittest 87.41% <ø> (+<0.01%) ⬆️

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

see 1 file with indirect coverage changes

Improve wording for the compose/pipe example.
@ericelliott
Copy link
Author

Thanks for the quick attention on this @JoshuaKGoldberg @Josh-Cena @bradzacher . 🎉

I think I have addressed all of the concerns.

Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with the current wording :)

@bradzacher
Copy link
Member

This checklist discourages people from contributing PRs by trying to force us to wait around for "accepting prs". If you don't want to accept a PR, just close it.

This is on purpose - we don't want people to spend hours or days doing work that we won't accept into the project - it wastes their time and deters them from making future contributions.

We've had a number of people in the past raise PRs for new options or docs changes that we as maintainers have disagreed with in the context of the project. It feels sucky for us to have to close a PR that someone was excited about and spent time on, and it feels even worse for the author as well.

Additionally the default search on the issues tab only includes issues. So if you just raise a PR and we discuss it within the PR then people won't easily be able to discover the discussion. This increases the chance of duplicates and the like.
This also has the benefit of focusing the discussion in the PR - the issue is for discussing the idea and whether or not it makes sense, then the PR is for reviewing the change itself.


TL;DR by asking for an issue first it means

  1. the discussion is easily discoverable in future,
  2. the threads can be more focused, and
  3. the contributor is less likely to "waste" time on work that's in the wrong direction.

ericelliott and others added 2 commits July 29, 2023 20:27
Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Jul 30, 2023

@ericelliott in this PR you have written us over 500 words -most of which are in the latest few posts in #7355 (comment) - and linked articles & papers containing well over 10,000 more explaining your points. I truly appreciate and respect your enthusiasm and willingness to espouse best practices and work with us so we understand where you're coming from. ❤️

But:

  • Per our contributing guidelines and @bradzacher's comment just above this one (Update no-explicit-any.md #7355 (comment)), our PRs are not a good place for this kind of deep discussion.
  • Per Update no-explicit-any.md #7355 (comment), the conversation is devolving into a much grander scope than it needs to be for this issue.
  • Although it's lovely that you have the time to get into deep debates/discussions on this on the internet, neither our maintainer time nor the time of most of our prospective contributors is likely well spent getting dragged into them. Having to read through hundreds of comment words + skim -let alone deeply read- thousands of words of academic papers to participate in a repository's discussion is a significant barrier to entry.

I'm going to close this PR. If you want to discuss specifically the phrasing around how to say that any is a sometimes-necessary, dangerous escape hatch then great!, let's do it in #7354.

If you want to bring up functional programming theory, theory of type safety, or any other grandiose topic, I would encourage you to do that separately from here.

@JoshuaKGoldberg JoshuaKGoldberg added blocked by another issue Issues which are not ready because another issue needs to be resolved first wontfix This will not be worked on and removed awaiting response Issues waiting for a reply from the OP or another party blocked by another issue Issues which are not ready because another issue needs to be resolved first labels Jul 30, 2023
@ericelliott
Copy link
Author

@JoshuaKGoldberg I believe your statement "any is a sometimes-necessary, dangerous escape hatch" resolves the discussion beautifully, and I'm willing to amend the PR to say that. My concern was that people see an absolute, unqualified statement like "any is dangerous" and feel like they need to shoot themselves in the foot to eliminate any.

I agree this is not the right forum for the deep debate, and it was never my intention to debate anybody.

That said, the suggested changes (with your excellent addition of "any is a sometimes-necessary, dangerous escape hatch") seem like a great fix for the issue.

The PR is now closed, I can't conveniently make the change. I don't feel like my contributions are welcome or appreciated at this stage. I'll leave you to it.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: add allowance in no-explicit-any docs that any is sometimes a necessary last resort
4 participants