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

Build should error with config that has strictNullChecks: false #152

Open
nygrenh opened this issue Sep 10, 2021 · 6 comments
Open

Build should error with config that has strictNullChecks: false #152

nygrenh opened this issue Sep 10, 2021 · 6 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed pr requested

Comments

@nygrenh
Copy link

nygrenh commented Sep 10, 2021

Given the following interface:

export interface Example {
  name: string | null;
}

If I generate a type guard for this, it creates the following function:

import { Example } from "./test";

export function isExample(obj: any, _argumentName?: string): obj is Example {
    return (
        (obj !== null &&
            typeof obj === "object" ||
            typeof obj === "function") &&
        typeof obj.name === "string"
    )
}

Now if call the type guard like this:

const obj: Example = { name: null }
console.log(isExample(obj))

The output is false, but I'd expect this to output true.

@rhys-vdw
Copy link
Owner

@nygrenh hm... so there's no specific test cast for union type with null. If this is indeed the current behaviour then it's a terrible bug, and likely a regression. Can you confirm whether you have strictNullChecks enabled?

@nygrenh
Copy link
Author

nygrenh commented Sep 10, 2021

strictNullChecks was not enabled. If I switch it on, the output changes to the correct form:

export function isExample(obj: any, _argumentName?: string): obj is Example {
    return (
        (obj !== null &&
            typeof obj === "object" ||
            typeof obj === "function") &&
        (obj.name === null ||
            typeof obj.name === "string")
    )
}

@rhys-vdw
Copy link
Owner

@nygrenh this is known behaviour. There is a conceivable way that you could generate guards with strictNullChecks disabled, it would basically mean allowing any field or object to be null or undefined.

We could forcibly override this option before ts-auto-guard runs. However that would give incorrect results too. For example:

import { isMyInterface } from "./MyInterface.guard.ts"

// without null checks
interface MyInterface {
  value: int;
}

let valueInt: MyInterface = { value: 5 }; // valid
let valueUndefined: MyInterface = { value: undefined }; // valid
let valueNull: MyInterface = { value: null }; // valid

isMyInterface(valueInt); // true
isMyInterface(valueUndefined); // incorrectly returns false
isMyInterface(valueNull); // incorrectly returns false

But if we go the other way and permit any value to be undefined or null it's a lot of noise and the end validators are mostly useless. So it's not really clear how to handle this setting.

Generally speaking the goal of this project is to create type checks for people who want strict types, so the intersection with people working with strictNullChecks disabled is presumably small.

See #120, see #99

I think the solution is to warn or error when the program runs.

@rhys-vdw rhys-vdw changed the title Type checking fails for for a field that is either a string or null Type checking fails for for a field that is either a string or null with strictNullChecks: false Sep 10, 2021
@rhys-vdw rhys-vdw mentioned this issue Sep 10, 2021
@rhys-vdw rhys-vdw added enhancement New feature or request bug Something isn't working and removed more information required enhancement New feature or request labels Sep 10, 2021
@nygrenh
Copy link
Author

nygrenh commented Sep 10, 2021

Yeah, I actually had strictNullChecks off only by accident -- was setting up a new project and didn't notice it.

I agree that a warning / error would be a good solution to this.

Thanks for your help!

@rhys-vdw rhys-vdw added good first issue Good for newcomers help wanted Extra attention is needed labels Sep 13, 2021
@rhys-vdw
Copy link
Owner

Marking this issue as "help wanted". Further discussion notwithstanding, the accepted solution will error the build if structNullChecks is false. This must correctly handle the case where strict: true is enabled and strictNullChecks is not provided.

@rhys-vdw rhys-vdw changed the title Type checking fails for for a field that is either a string or null with strictNullChecks: false Build should error with config that has strictNullChecks: false Sep 13, 2021
@rhys-vdw
Copy link
Owner

Further thoughts... The ideal behaviour here would be for ts-morph to handle this naturally by including null or undefined as a possible value for every single type with strictNullChecks disabled. I suspect this is the behaviour of typescript compiler itself and may be undesirable for other use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed pr requested
Projects
None yet
Development

No branches or pull requests

2 participants