New rule: prefer-readonly-type-declaration

[RebeccaStevens]

Re #153

[RebeccaStevens]

@jonaskello I think this implementation addresses all the issues and is pretty much ready to go.

I copied over the upstream utils that we are waiting on for now. It can easily be stripped out later.

There are a few changes I want to make to those utils but I'll submit upstream PRs to try and address those.

Let me know what you think.

[jonaskello]

Regarding the name of the rule, I'm thinking maybe prefer-readonly-declaration would suffice to keep it shorter. However the longer name is more correct so I'm not sure about this.

Regarding the new aliases option I think we need to perhaps discuss this a bit further and think about use-cases and overlaps with the ignorePattern option. If the intent of the rule is to require that every declaration is readonly save a few exceptional cases I think the ignorePattern option already solves this. Or is there some use case where the aliases option would be needed to have full coverage for this use-case?

[jonaskello]

If I understand correctly this rule will no longer check readonly declarations of function parameters but instead rely on the @typescript-eslint/prefer-readonly-parameter-types for that, which is good because we don't need to overlap what is already present in the offical lib. I saw this was noted in the deprecation notes. I think it should probably be noted in the docs for this rule too that this is an intentional omission.

[RebeccaStevens]

Rule name

I think having the word type in the name makes it more clear that the rule is for types and thus a TypeScript specific rule. This also makes it clear that rule doesn't check other declarations (such as function declarations or variable declarations) (to be clear, it does check type declarations on these other declarations).

Aliases option

This option is more powerful than the ignorePattern option but it only applies to type aliases and interfaces.
The main thing it allows for is checking whether a deeply nested property is allowed to be mutable or not based on the alias's/interface's name. The ignorePattern option only checks shallowly against the property's name.

It's probably also worth noting that this new rule is now more strict than the previous one. Type aliases/interfaces are also checked as whole now, rather than the individual elements being individually checked. (The individual elements are also check but only for the purpose of the fixer - this is simply because implementing a fixer for the whole type would be more difficult and we already had the code to do it this way. I may implement the other way in the future.)

I could maybe replace the aliases options with options something like this:

{
  "readonlyAliasPatterns": "^(?!I?Mutable).+$",
  "mutableAliasPatterns": "^I?Mutable.+$",
  "blacklistedAliasPatterns": "^Mutable$",
}

Function parameters

this rule will no longer check readonly declarations of function parameters but instead rely on the @typescript-eslint/prefer-readonly-parameter-types for that

That's actually not correct. This rule will still check function parameters just like before if they use a type literal (as opposed to a type reference).

For example:

// Will fail to both "prefer-readonly-type-declaration" and "@typescript-eslint/prefer-readonly-parameter-types"
function foo(param: { bar: number }) {}

// Will fail to "prefer-readonly-type-declaration"
type Param = { bar: number };

// Will fail only to "@typescript-eslint/prefer-readonly-parameter-types"
function foo(param: Param) {}

The main reason I kept this behavior was so that we can still implement our fixer (to add readonly modifiers to the properties and change Array to ReadonlyArray, etc).

[jonaskello]

Rule name

Yes, it is definitely a tradeoff between brevity and clarity. I have no strong opinion on this so let's go with the longer name.

Aliases option

I will have to think about this a little more but just a quick question on terminology for now. If we write eg. type MyTuple = [number, string] I think that MyTupe is referred to as a "type alias". When you say "...only applies to type aliases..." I think you mean only type aliases for object/record types (ie. types that have fields)? Or is the intent to also check other aliases, eg. if you alias an array or tuple?

Function parameters

Ah, I did not catch that, thanks for clarifying. Good point about the fixer, don't want to be without that :-).

[RebeccaStevens]

type MyTuple = [number, string] will be checked by this rule and flagged.

Things like type MyType = typeof myValue; will also be checked.

[RebeccaStevens]

Oh, I should point out that the this new rule also supports Readonly<T>.

[jonaskello]

Yes, type MyTuple = [number, string] will be flagged by the old rule too I think? What I'm trying to figure out is if the intent behind the aliases option to be able to do something like this:

type MutableTuple = [string, number] // This is is ignored using `alias` option? 
type MyTuple = [string, number] // This is flagged

or if it is more geared towards something like this:

type MutableObj = { x: number, y: {z: string}  }; // This is is ignored using `alias` option? 
type MyObj = { x: number, y: string }; //  This is flagged

or the intent is to support both these cases?

[RebeccaStevens]

To support both.

[jonaskello]

Just to clairfy a bit about the ignorePattern option. The intent of this rule was to mimic how some functional-first languages work. Everything is immutable except where explicitly marked immutable mutable. AFAIK these languages provides two escape hatches to make things mutable (eg. for performance reasons). The first one is to make a variable mutable, we can look at this example for F#:

let x = 15
x <- 15 // this will not work
let mutable y = 10
y <- 15 // this will work since y is marked as mutable

The equivalent in js/ts is const vs let so here we use the prefer-const rule with ignorePattern escape hatch to name the variable something to emulate the mutable modifier.

let x = 15 // this will not work becuase of prefer-const rule
let mutableY = 10 // this will work since y is named as mutable and is ignored by prefer-const rule

The second escape hatch these languages provide is using the same modifier but on fields within a record type. The F# example link as above contians this example:

type studentData =
   { ID : int;
      mutable IsRegistered : bool;
      mutable RegisteredText : string; }

In this case ID is immutable but IsRegistered and RegisteredText are mutable since they are marked with the mutable modifier. The same thing in typescript with prefer-readonly-type rule and ignorePattern option:

type studentData =
   { readonly ID : number;
      MutableIsRegistered : boolean;
      MutableRegisteredText : string; }

AFAIK the functional languages the rule is trying to emulate does not have an escape hatch that allows a whole type to be marked as mutable. You can of course mark every field with the modifier, and the equivalent in typescript with the rule would then be to prefix all field names with mutable. My concern is that I'm not sure it's a good idea to provide escape hatches for our rules that do not exist in functional-first languages.

[jonaskello]

OTOH, typescript is really different from F#/ocaml/reasonml etc. so it might make sense to have more escape options :-).

I guess the aliases option that enable whole type mutability would be like having the mutable modifier on the type keyword in F# etc. Something like this:

type mutable studentData =
   { ID : int;
      IsRegistered : bool;
      RegisteredText : string; }

I don't think this syntax exists in F# or other ML based languages but if it did I guess it would make all fields of the type studentData mutable. Not sure why they avoid this syntax in those languages.

[jonaskello]

I'm trying to think of use-cases for the aliases option. I'm mainly thinking from the perspective of making immutable the default and then having some way to ignore checking where it is not wanted. I've come up with the following so far:

// This works already with prefer-readonly-type and ignorePattern set to "^[mM]utable"
export const mutableNumbersArray: number[] = [1, 2, 3];
export type Foo = { mutableBar: number };
export type MutableNumbersArray = number[];
export const numbersArray: MutableNumbersArray = [1, 2, 3];
// This does not work with ignorePattern
export type MutableFoo = { bar: number };

So the case I have found so far is to mark a whole object type alias as mutable. Are there more cases?

[RebeccaStevens]

Fully functional programming languages such as Haskell don't even have the concept of mutability. Although F# allows for allowing for mutability, this is just an escape hatch into non-functional programming.

I agree that we should try to support the same style of marking properties as mutable that F# does (via a naming convention); but we shouldn't simply lock into this being the end all. And yeah, as TypeScript is a very different language to F# and the like, enabling a way to mark an entire type as mutable seems like it should be supported - especially when working with 3rd-party libraries. In fact, our code is relying on this to work with ESLint see type MutableRuleTesterTests in this commit

[RebeccaStevens]

I just reworked the aliases options to make them more simple but just as powerful.

Now instead of the aliases object, there are these 3 options: ignoreAliasPatterns, readonlyAliasPatterns and mutableAliasPatterns.

[RebeccaStevens]

Also note that with the default config and ignorePattern set to "^[mM]utable", the following would actually be a violation.

// { "ignorePattern": "^[mM]utable" }
type Foo = { mutableBar: number };
//   ^^^ Error - name suggests the type should not be mutable.

To fix this the type's name would have to be renamed:

// { "ignorePattern": "^[mM]utable" }
type MutableFoo = { mutableBar: number };
//                  ^^^^^^^^^^ No error but the mutable prefix has no effect.

Or the readonlyAliasPatterns would need to configure differently. The option can be turned off by setting it to [] and this would restore the previous behavior of prefer-readonly-type.

// { "ignorePattern": "^[mM]utable", readonlyAliasPatterns: [] }
type Foo = { mutableBar: number }; // Works just like it did with `prefer-readonly-type`.