Feature Request: Readonly<T> should remove mutable methods

rope-hmg commented 4 years ago

Search Terms

readonly, as const, Readonly,

Suggestion

Readonly to work the way Readonly<T[]> does without having to create something like ReadonlyArray, ReadonlySet, etc...

I would love it if the Readonly<T> mapped type could exclude any method marked as readonly or maybe as cosnt or something similar.

Use Cases

At the moment I have to do this:

class Vector2 {
    public constructor(
        public x: number,
        public x: number,
    ) {}

    public add(other: ReadonlyVector2): Vector2 {
        return this.clone().addInPlace(other);
    }

    public addInPlace(other: ReadonlyVector2): this {
        this.x += other.x;
        this.y += other.y;
        return this;
    }

    public clone(): Vector2 {
        return new Vector2(this.x, this.y);
    }
}

interface ReadonlyVector2 {
    readonly x: number;
    readonly y: number;
    add(other: ReadonlyVector2): Vector2;
    clone(): Vector2;
}

This gets very boring with a lot of types, also it's error prone. I could forget to add something to the interface, or worse I could accidentally add something that is mutable.

I can currently do method(this: Readonly<this>) {...}, which is helpful, but doesn't stop me calling mutable methods on the type.

Inside a marked method you would be unable to mutate the state of member variables and you would unable to call other unmarked methods.

This would also allow you to remove the special case of Readonly<T[]>. The mutable methods of arrays could be marked with whatever syntax is decided and then Readonly<T[]> would just work like any other Readonly<T>. Currently I don't bother using Readonly<T> since it doesn't really help with anything except C style POD types.

Examples

class Vector2 {
    public constructor(
        public x: number,
        public x: number,
    ) {}

    public readonly add(other: Readonly<Vector2>): Vector2 {
        return this.clone().addInPlace(other);
    }

    public addInPlace(other: Readonly<Vector2>): this {
        this.x += other.x;
        this.y += other.y;
        return this;
    }

    public const clone(): Vector2 {
        return new Vector2(this.x, this.y);
    }
}

Checklist

My suggestion meets these guidelines:

[x] This wouldn't be a breaking change in existing TypeScript/JavaScript code
[x] This wouldn't change the runtime behavior of existing JavaScript code
[x] This could be implemented without emitting different JS based on the types of the expressions
[x] This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
[x] This feature would agree with the rest of TypeScript's Design Goals.

fatcerberus commented 4 years ago

How does the compiler know which methods are mutative and which ones aren’t?

AnyhowStep commented 4 years ago

Remove all the methods!

rope-hmg commented 4 years ago

It knows because you mark the non mutating methods.

class Foo {
    private state = 1;

    mutateState() {
        this.state += 1;
    }

    readonly getState() {
        // Inside a readonly method this is treated as Readonly<this> which will mark all
        // properties as readonly and only all you to call readonly methods.
        return this.state;
    }
}

Don't worry about the syntax at the moment. I don't know what would be best for that. Right now I just want to discuss the idea.

fatcerberus commented 4 years ago

It’s a good idea, but I’d expect it to work something like “const correctness” in C/C++—that is, non-mutating methods should not be able to call anything mutative, and this should be verified by the compiler. Otherwise the annotation (whatever it ends up being) has no teeth and would be easy to get wrong.

rope-hmg commented 4 years ago

I could have a go at making a PR if that would be useful?

rope-hmg commented 4 years ago

I've been experimenting with some code and I've refined my pattern some more. So the following is how it works today:

export class Rectangle {
    public constructor(
        private width: number,
        private height: number,
    ) {}

    public getArea(this: InternalReadonlyRectangle): number {
        return this.width * this.height;
    }

    public setWidth(value: number): void {
        this.width = value;
    }

    public setHeight(value: number): void {
        this.height = value;
    }
}

interface InternalReadonlyRectangle {
    readonly width: number;
    readonly height: number;

    getArea(): number;
}

export interface ReadonlyRectangle {
    getArea(): number;
}

This is not bad, but it does leave some things to be desired.

You have to create multiple interfaces if you want to have readonly private members.
It's a lot of boiler plate to write for every type.

I would like to be able to write this:

class Rectangle {
    public constructor(
        private width: number,
        private height: number,
    ) {}

    public getArea(this: Readonly<this>): number {
        return this.width * this.height;
    }

    // Some syntax ideas:
    // readonly public getArea(): number
    // const public getArea(): number
    // public getArea(this as const): number
    // Rust style:
    // public getArea(Readonly<this>): number 
    // Or just what we have today. I'm not too bothered really.

    public setWidth(value: number): void {
        this.width = value;
    }

    public setHeight(value: number): void {
        this.height = value;
    }
}

rope-hmg commented 4 years ago

Two more things that I have either noticed, or thought about:

Should the readonly be deep? Personally I think yes. Like the as const.
With the current code, if something is only read in a readonly context, then TypeScript complains that the value is never read. This is caused by using this: ReadonlyFoo in the readonly methods. TypeScript doesn't know that they're the same property.

kwasimensah commented 4 years ago

Note method(this: Readonly) won't work across inheritance.

class Foo {
    x: number = 0;
    test(this: Readonly<Foo>){
        // this.x  = 5; will raise error.
        console.log(x);
    }
}

class Bar extends Foo {
    test(){
        this.x  = 5; // Compiler won't complain.
    }
}

OldStarchy commented 2 years ago

getArea(this: Readonly<this>): number is a bit too wordy for my preference. I would prefer something like const getArea(): number, and I would prefer const over readonly as it may help with understanding if people are familiar with the concept in c++. Though readonly would be more consistent with typescript.

https://stackoverflow.com/a/751783

It may be worth thinking about the idea of mutable too, I'd never heard of it before reading that answer though I'm not a C++ developer. There is an issue about it already #24509.

Also, I don't think this would be a breaking change if the const foo(): void syntax was used, though it would be a breaking change for a library to start using it.

rope-hmg commented 2 years ago

Another blocker with the method(this: PrivateReadonlyWhatever): void is:

You can't model private properties with an interface because the properties in an interface are public.

rope-hmg commented 2 years ago

@OldStarchy Using const might be a better idea anyway since as const is deep and I would like this feature to also be deep.

What I mean by that is:

class Thing {
    private value = { prop: { inner: 0 } };

    setValue(newValue: number): void {
        this.value.prop.inner = newValue;
    }

    const getValue(): number {
        // This is an error because the method is marked as const so this.value has type { readonly prop: { readonly inner: number } }.
        // Similar to how `as const` acts on object literals.
        this.value.prop.inner = 15;

        return this.value.prop.inner;
    }
}

viridia commented 1 year ago

While I like this idea, it would break backwards compatibility pretty badly. Currently Readonly does not exclude any methods, the proposal as stated would exclude all of them by default.

A backwards-compatible alternative is that instead of marking non-mutating methods as readonly, you would mark mutating methods as mutable. Only methods so marked would be excluded. This would allow existing codebases to continue to compile, and the mutable modifier could be added incrementally as desired.

rope-hmg commented 1 year ago

My proposal is backwards compatible because marking a method as const is opt-in. Once you do, you will break any code that was assuming it could call any method on a Readonly<T>. If your code was a library this change would require a major version bump since it'd be a semver breaking change. If you're code is an application then you just have to refactor all the use-sites.

What you've proposed wouldn't be backwards compatible since it would make all methods const by default. It's probably not too crazy to say the majority of methods on a class in any given codebase are mutable. Marking mutable methods would mean that these codebases would have to annotate a large portion of their methods. With the change I'm proposing mutable would be the default as it is now.

Your proposed change would make the following an error:

class Something {
    private counter: number;

    public doThing(): void {
        this.counter += 1;
//      ^^^^^^^^^^^^^^^^^
//      Error: Can't mutable `this.counter` in non-mutable method `doThing`.
//      Mutable methods must be marked as `mutable`.
    }
}

The above would have to be the case otherwise there would be no guarantee that a plain method (i.e. one not marked as mutable) doesn't actually mutate the class. This guarantee is required since you want to remove mutable methods when using Readonly<T>. Consider the following:

const a: Readonly<Something> = new Something();

a.doThing();
// There would be no error here, even though there should be, since `doThing` is not
// marked `mutable` and therefore not remove from `Readonly<Something>`.

Personally, I would prefer immutable by default, but it would cause a lot of headaches to change retroactively.

microsoft / TypeScript