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

New Feature Requesting for const Parameter #18497

Open
lihz6 opened this issue Sep 15, 2017 · 39 comments
Open

New Feature Requesting for const Parameter #18497

lihz6 opened this issue Sep 15, 2017 · 39 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@lihz6
Copy link

lihz6 commented Sep 15, 2017

I have a small new feature to request. Let say we have this piece of code:

function setButton(button: Button | undefined) {
    if (button) {
        button.onclick = () => {
            // tsc complains button might be undefined(well done!), 
            // because it might be set to undefined later on,
            // as you can see below `button = undefined;`. 
            button.setAttribute('disabled', 'true'); //error
        }
    }
    // some other code goes here...
    // and eventually, button is set to undefined.
    button = undefined;
}

Fortunately, there is a way to fix this:

function setButton(button: Button | undefined) {
    const constButton = button;
    if (constButton) {
        constButton.onclick = () => {
            // now tsc is satisfied.
            constButton.setAttribute('disabled', 'true'); 
        }
    }
    
    // impossible to assign to a const variable.
    // constButton = undefined;
}

But can we twist the code a little bit, which is what I asking for? Something like this:

function setButton(const button: Button | undefined) {
    if (button) {
        button.onclick = () => {
            button.setAttribute('disabled', 'true'); 
        }
    }
}

So you can see there really are some cases a const parameter might come to handy. Thank you.

P.S. Just for demonstrating the point, here's a full demo:

class Button {
    onclick?: () => void;
    performClick() {
        if (this.onclick) this.onclick();
    }
    setAttribute(key: string, value: string) {
        //set attribute to this button
    }
}

function setButton(button: Button | undefined) {
    if (button) {
        button.onclick = () => {
            button.setAttribute('disabled', 'true');
        }
    }
    button = undefined;
}

const button = new Button();
setButton(button);
button.performClick();
@lihz6 lihz6 changed the title Requesting for const parameter New Feature Requesting for const Parameter Sep 15, 2017
@kitsonk
Copy link
Contributor

kitsonk commented Sep 15, 2017

This is essentially covered by #17502 and related to #9741 specifically this comment

@lihz6
Copy link
Author

lihz6 commented Sep 15, 2017

@kitsonk Oh, thanks. I see, and let's say this is another useful example, which affects the way we code.

@mhegazy mhegazy added the Suggestion An idea for TypeScript label Sep 15, 2017
@ghost
Copy link

ghost commented Sep 15, 2017

tslint has a rule for this.

@RyanCavanaugh RyanCavanaugh added the Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature label Sep 15, 2017
@lihz6
Copy link
Author

lihz6 commented Sep 16, 2017

@andy-ms That may be a little overdo, since there are some other parameters need reassignment.

@brn
Copy link

brn commented Sep 17, 2017

I think better to use readonly keyword for this proposal.

function foo(readonly constantProperty: any) {
  constantProperty = 1 // Error. Couldn't assign a readonly parameter.
}

@RyanCavanaugh
Copy link
Member

@ngolin tslint has options for overriding errors. I can't imagine this is common enough for that to be burdensome?

@yxliang01
Copy link

yxliang01 commented Sep 28, 2017

The no-parameter-reassignment rule of tslint I would say is not as flexible as what this issue proposes because what the rule does is that we have no choice but enforcing const on all parameters which is quite unfavourable. So, highly recommend implementing this.

@ghost
Copy link

ghost commented Sep 28, 2017

@yxliang01 You can add // tslint:disable-next-line no-parameter-reassignment in front of any function that needs mutable parameters. But keep in mind that reassigning the parameter to a function does not change the value for the caller, so it's equivalent to creating a mutable local variable initialized to the parameter.

@yxliang01
Copy link

Thanks @andy-ms . However, it's still not ideal... I think it will make the code messier. Also, there's no control over individual parameters...

@ajafff
Copy link
Contributor

ajafff commented Oct 1, 2017

@brn readonly can be used to declare parameter properties in constructors. Changing that behavior is a breaking change. And changing the effect of readonly depending on the containing function/method/constructor is not a good thing either.

@yxliang01
Copy link

@ajafff why should it be a tslint rule instead of a declaration like what other languages do?

@aluanhaddad
Copy link
Contributor

@yxliang01 readonly on a constructor parameter has a substantive meaning for a declaration. Everything discussed here would be erased from declarations and would only be visible to source level consumers. Even at that point, it's just noise, completely meaningless, except during the period of time that you are editing the the body of the specific function declaring the parameter.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 2, 2017

Even at that point, it's just noise, completely meaningless, except during the period of time that you are editing the the body of the specific function declaring the parameter.

That could be argued about the whole type system. 😉 It is very arguable that re-assignment of parameters is something that an end user would want to control, as it can lead to logic errors in the code, and is the main goal of TypeScript.

On the other hand, I would much rather see #17502 more fully addressed, because it feels like is addressing the problem space versus a point feature.

@aluanhaddad
Copy link
Contributor

I meant it is noise in the interface declaration. since mutation of the parameter inside the method, which is a terrible practice, does not affect the caller the information is not useful. so if I'm consuming something from source and I see a bunch of annotations on parameters stating that their immutable but I'm not changing the code of those functions the information is not helpful to me. I suppose it could be argued that its presence indicates potentially higher quality or more thoughtfully written functions.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Oct 2, 2017

everytime I look at some Java code, which fortunately isn't very often, they strike me as implementation details exposed in the same location as a functions interface. this doesn't apply to type information in general. It does apply to parameter destructuring, which is also noise for the source level consumer, but I digress...

Anyway, Scala got this one right.

@ajafff
Copy link
Contributor

ajafff commented Oct 2, 2017

@yxliang01 I'd like to see this as part of the language. I just don't think readonly should be used for it, because it already has a different meaning.

Until this is implemented in TypeScript, you can use the lint rule I just wrote: https://github.com/ajafff/tslint-consistent-codestyle/blob/master/docs/const-parameters.md. In contrast to tslint's no-parameter-reassignment you enable it for individual parameters using JsDoc /**@const*/

@Conaclos
Copy link

Conaclos commented Mar 4, 2018

Most functions do not assign their parameters. A systematic use of const or readonly for each formal parameter seems too verbose. Who really uses the final keyword in Java for formal parameters?

I think this kind of assignment should be impossible. However, this could break a lot of code. A compiler option seems the best option... or the use of a linter?

@Zorgatone
Copy link

I certainly need this one! 👍

@lihz6 lihz6 closed this as completed Nov 12, 2018
@lppedd
Copy link

lppedd commented Nov 16, 2018

@Conaclos

Who really uses the final keyword in Java for formal parameters?

I put final everywhere, and everyone should do that.
And lately, that's why I switched to Kotlin, final by default for arguments.

@Conaclos
Copy link

@lppedd
I use final as much as possible except on formal parameters. Because I'm using only read-only parameters, it could be very verbose to mark every formal parameter as such. IDE and linters often offer a setting to enforce it. Moreover, in most of source codes, formal parameters are read-only. Thus, I think that programmers consider them as read-only.

Nice to know that Kotlin do that right.

@0Ams
Copy link

0Ams commented Nov 21, 2018

Are these issues reflected in typescript? Or should we use only tslint? I hope these functions are supported by typescript. But I wonder if the issue is closed and is not being discussed anymore

@cyyyu
Copy link

cyyyu commented Jan 9, 2019

Searching around google and got here.

I feel like this should be part of ts instead of tslint. Is it still under discussing?

@ghost
Copy link

ghost commented Jul 6, 2019

I really want this

@cheapsteak
Copy link

cheapsteak commented Nov 27, 2019

I have a need for this edit: I stand corrected!


I think my use case is slightly different from the ones described above

I have a function with a type signature similar to this (much simplified)

const fn = <T> (x: T) => ({ key: x as T });

When I call fn with a string, e.g. fn('someString'), I need for the returned type to be "someString" rather than being widened to string, because I'm using the key as the key in a map, and I want typescript to know all the keys on that map so access to that map can be type-safe

I would like a way to enforce that fn is being called with a const otherwise the type data on the map is no longer accurate

Something like

const fn = <T> (const x: T) => ({ key: x as T });
// or
const fn = <T> (readonly x: T) => ({ key: x as T });

would be great

@weswigham
Copy link
Member

@cheapsteak

type UnitsOnly<T> = string extends T ? never : T;
const fn = <T extends string>(x: UnitsOnly<T>) => ({ key: x });

fn("ok");
fn("not ok" as string);

Playground Link

@ShacharHarshuv
Copy link

I think this is a good idea.
Backing people in other comments - the "readonly" keyword is a good idea.
This is sort of like what C++ have.

@elegon32
Copy link

I want this. Why is this closed?

@hitallow
Copy link

hitallow commented Apr 2, 2020

this is an excellent idea, I really want this now.

@snebjorn
Copy link

@ngolin could you please reopen this?

@thw0rted
Copy link

thw0rted commented Sep 2, 2020

I've read this, and skimmed #17502, as well as #9741. I don't think my argument is covered anywhere else, and I think it's a reasonably good one.

I use this pattern a lot:

function arrFind(obj: string[], term?: string) {
  if (!term) { return DEFAULT_RESULT; }
  return obj.find(x => x.includes(term));
}

This will fail because the includes method doesn't accept undefined. Now, we already narrowed away undefined with the early return statement, but it doesn't matter because term isn't technically const, even if our linter yells at us for assigning to it.
That means all narrowing is lost inside the arrow function. What I wind up doing in practice is

function arrFind(obj: string[], term?: string) {
  const searchTerm = term;
  if (!searchTerm) { return DEFAULT_RESULT; }
  return obj.find(x => x.includes(searchTerm));
}

This is added verbosity for the sake of making the compiler happy, and I probably did it 200 times in my current project. If I could simply declare the parameters as const, or better yet if const was the default behavior (as it should be!), I could get rid of all those pointless aliases.

@antoine-pous
Copy link

antoine-pous commented Dec 4, 2020

I don't see a reason for #18497 to have been closed, so I've reopened it.

Thanks for reopening it 👍

That said, this can be implemented by a lint rule (e.g. make all params consty by default, have an opt-out through some mechanism), so the bar is a bit higher in terms of whether we would do it or defer to eslint.

@RyanCavanaugh i understand, from my point of view TS is more reliable for this implementation than using an ESLint rule, maybe am I wrong 🤔

@thw0rted
Copy link

thw0rted commented Dec 4, 2020

I don't understand how a lint rule would make the OP's example code work without introducing an extra variable, as in the second code block (constButton). If it's not possible to declare formal params as const, no amount of linting can (or should!) make the typechecker apply narrowing in a nested scope.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Dec 4, 2020

The thing we've discussed (offline?) is just having a setting for "all parameters are always const", since in practice it isn't much of a burden to always write code that way. Introducing an actual inline const keyword is unlikely to happen due to this being an area clearly in TC39's domain.

@chase-moskal
Copy link

chase-moskal commented Feb 18, 2021

@RyanCavanaugh

unlikely to happen due to this being an area clearly in TC39's domain.

why not keep readonly in typescript's domain? like this

function awesome(x: readonly number) {}

and i'd love to have a flag, like defaultReadonlyArguments, which makes all arguments readonly by default. then we need a way to mark arguments as mutable — we flip it and reverse it!

function awesomer(x: mutable number) {}

@minecrawler
Copy link

I love the idea of making things immutable by default. From RustLang we already know that it's a good idea to make good design choices the simplest way to code.

However, this is TypeScript, which want to appeal to JavaScript devs who want to take their coding to the next level. Switching the default behavior around sounds like inviting trouble and many lost developers, plus the tsc does not output good enough error messages and suggestions to guide people.

That's why I would love to see some way to mark parameters as const or readonly, something which is already known from other places in TypeScript.

@connormckelvey
Copy link

connormckelvey commented Mar 8, 2022

I have a use case for which I would really like this sort of functionality built into the type-checker. TsLint might be good for linting one's own code, but the checks get lost once build/bundled/distributed.

For my use case, I am developing a library that does some state management on behalf of a user. The user interacts with the state in a restricted way so that the library knows to check the state for changes. For example, imagine:

class StateManager<S = any> {
    constructor (private readonly state: S) {}
    
    public updateState(updater: (state: S) => void) {
        updater(this.state)
        // do stuff on state change
    }
}

// BAD
stateManager.updateState((state) => {
    state = { different: "state" }
})

// OK
stateManager.updateState((state) => {
    state.prop = "update"
    state.nested.prop = 2
})

The bad use case could be avoided with a const/readonly/final type annotation:

class StateManager<S = any> {
    constructor private state: S
    
    public updateState(updater: (readonly state: S) => void) {
        updater(this.state)
        // do stuff on state change
    }
}

@OmgImAlexis
Copy link

@connormckelvey TSLint has been deprecated as of 2019

@thw0rted
Copy link

thw0rted commented Mar 8, 2022

Connor might have meant @typescript-eslint, which I believe does have rules about mutating parameters. I don't think that really matters for his use case, though, because as a library author you have no control over how your code is consumed. TS is a strong ecosystem but it's still dwarfed by vanilla JS. If your consumer isn't using TS, there's nothing to stop the "bad" pattern in your example. That's not to say there's no point to this issue -- it certainly would be more ergonomic for TS consumers to get an error if they try to reassign the parameter.

@flowstate247
Copy link

@RyanCavanaugh

The thing we've discussed (offline?) is just having a setting for "all parameters are always const", since in practice it isn't much of a burden to always write code that way. Introducing an actual inline const keyword is unlikely to happen due to this being an area clearly in TC39's domain.

👍
That's what Kotlin chose to do too. https://blog.jetbrains.com/kotlin/2013/02/kotlin-m5-1/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.