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

set but no get on class... but using get on instance of class compiles. #30852

Closed
rmbar opened this issue Apr 10, 2019 · 8 comments
Closed

set but no get on class... but using get on instance of class compiles. #30852

rmbar opened this issue Apr 10, 2019 · 8 comments
Labels
Duplicate An existing issue was already created

Comments

@rmbar
Copy link

rmbar commented Apr 10, 2019

TypeScript Version: 3.3.3333

Search Terms:
typescript default get when only set present

namespace Controller
{
    export class MainController
    {
        private readonly _codeWorkflowState = new CodeWorkflowState();

        private handleInterpreterWorkerMessage(e: any) 
        {
            if(this._codeWorkflowState.hasNexToInterpret)
            {
                // bug? nextToInterpret accessor does not exist. No static error reported. newerCode's value is undefined (the value) at runtime.
                const newerCode: string = this._codeWorkflowState.nextToInterpret;
            }
        }
    }

    class CodeWorkflowState
    {
        private _nextToInterpret : string | null = null;

        set nextToInterpret(code: string)
        {
            this._nextToInterpret = code;
        }

        get hasNexToInterpret() : boolean
        {
            return this._nextToInterpret !== null;
        }
    }
}

Expected behavior:

Error: no 'nextToInterpret' property on class 'CodeWorkflowState'

OR

Error: default compiler supplied accessor 'nextToInterpret' has type undefined but assigning to type string.

Actual behavior:

Compiles fine. Variable of type string has value undefined at runtime.

Playground Link: https://www.typescriptlang.org/play/index.html#src=namespace%20Controller%0D%0A%7B%0D%0A%20%20%20%20export%20class%20MainController%0D%0A%20%20%20%20%7B%0D%0A%20%20%20%20%20%20%20%20%2F**%0D%0A%20%20%20%20%20%20%20%20%20*%20Tracks%20the%20current%20state%20of%20code%20moving%20from%20editor%2C%20to%20interpreter%2C%20to%20renderer.%0D%0A%20%20%20%20%20%20%20%20%20*%2F%0D%0A%20%20%20%20%20%20%20%20private%20readonly%20_codeWorkflowState%20%3D%20new%20CodeWorkflowState()%3B%0D%0A%0D%0A%20%20%20%20%20%20%20%20private%20handleInterpreterWorkerMessage(e%3A%20any)%20%2F%2F%20any%20because%20from%20outside%20TypeScript%20so%20can't%20enforce%20type%0D%0A%20%20%20%20%20%20%20%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20if(this._codeWorkflowState.hasNexToInterpret)%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%2F%2F%20bug%3F%20nextToInterpret%20accessor%20does%20not%20exist.%20No%20static%20error%20reported.%20newerCode's%20value%20is%20undefined%20at%20runtime.%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20const%20newerCode%3A%20string%20%3D%20this._codeWorkflowState.nextToInterpret%3B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%7D%0D%0A%20%20%20%20%20%20%20%20%7D%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20class%20CodeWorkflowState%0D%0A%20%20%20%20%7B%0D%0A%20%20%20%20%20%20%20%20private%20_nextToInterpret%20%3A%20string%20%7C%20null%20%3D%20null%3B%0D%0A%0D%0A%20%20%20%20%20%20%20%20set%20nextToInterpret(code%3A%20string)%0D%0A%20%20%20%20%20%20%20%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20this._nextToInterpret%20%3D%20code%3B%0D%0A%20%20%20%20%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20%20%20%20%20get%20hasNexToInterpret()%20%3A%20boolean%0D%0A%20%20%20%20%20%20%20%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20return%20this._nextToInterpret%20!%3D%3D%20null%3B%0D%0A%20%20%20%20%20%20%20%20%7D%0D%0A%20%20%20%20%7D%0D%0A%7D

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Apr 10, 2019
@RyanCavanaugh
Copy link
Member

TypeScript doesn't have a concept of writeonly properties. See #21759

@rmbar
Copy link
Author

rmbar commented Apr 11, 2019

Ok, so let's say I've implicitly created a getter. What is the return type of this getter? I would think it would have to be undefined since this is the value that is actually returned by using it (at runtime). So why is the result of that getter assignable to const newerCode: string ?

@rmbar
Copy link
Author

rmbar commented Apr 11, 2019

Also, I'm not sure the duplicate label fits. I'm not advocating that this usage pattern should be supported. I'm advocating that it be a compile error. The impact of emitting this code, for our application, was that we had a value of undefined assigned to a variable of type string. This defeating of the type system lead to unexpected crashes for us. I believe CSA has enough information to flag this assignment as illegal. I think it should.

@RyanCavanaugh
Copy link
Member

I'm advocating that it be a compile error.

What is "it" ? The existence of a setter without a getter, or an assignment to the property? The former would be a breaking change, the latter needs representation in the type system, which is effectively the same as having a writeonly modifier (since that's what we'd have to emit to a corresponding .d.ts)

@rmbar
Copy link
Author

rmbar commented Apr 12, 2019

Sorry, by "it" I mean the assignment to the field newerCode in:

const newerCode: string = this._codeWorkflowState.nextToInterpret;

I think that assignment should generate a static error. I don't see the immediate connection to the writeonly modifier because I'm not actually sure what that means. Maybe my view is simplistic, but in the context of my submitted code I see the world as follows:

  1. The single line above is (currently) a legal TypeScript statement in the context of the provided code.
  2. It is an assignment with a lhs and a rhs.
  3. During type assignment, we need to do a type assignment to the lhs variable declaration and the rhs expression.
  4. Type assign to the lhs variable string because of the annotation.
  5. Type assign to the rhs expression type undefined (because this is the value I observe at runtime). I assume all invocations of non-existent getters on a class yield the value undefined.
  6. rhs type undefined is not invariant nor covariant wrt type string therefore report static error.

Our of curiosity, currently, what is the type assigned to the rhs expression of the assignment in the example code?

@RyanCavanaugh
Copy link
Member

To the absolute highest extent possible, an implementation file and a generated declaration file need to behave identically. Knowing that, take this class

// a.ts
class A {
  set y(s: string) {
  }
}

And run it through tsc --d:

// a.d.ts
declare class A {
    y: string;
}

Note that it would be wrong to generate

    y: string | undefined;

because the class doesn't allow writes of undefined to y,

The correct declaration file would say

    writeonly y: string;

@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@jcalz
Copy link
Contributor

jcalz commented Aug 19, 2021

@RyanCavanaugh said:

The existence of a setter without a getter [producing a compiler error] ... would be a breaking change

Seems worth considering. With --strictPropertyInitialization enabled I am surprised when I can read a non-optional non-undefined-able property and get undefined. Similar to how #27899 is asking for static properties to get checked for initialization, I'm imagining this issue asking for accessor-implemented properties to also get checked this way:

class Foo {
    set prop(x: string) { }
    //  ~~~~ <--
    // Property 'prop' has no 'get' accessor and its 'set' accessor type does not include 'undefined'.
    // Either 
}

which could be silenced either by accepting undefined

class Bar {
    set prop(x: string | undefined) { } // no error
}

or by declare-ing the getter (which is not valid today)

class Baz {
    set prop(x: string) { } // no error
    declare get prop(): string; 
}

Of course this is a breaking change, but is setter-only code more likely to be intentional or a mistake?

In any case, people coming here should probably be aware of ESLint's accessor-pairs linter rule, since it might help them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants