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

core: refine typing of isObject<T> #12259

Merged
merged 5 commits into from
Mar 9, 2023
Merged

core: refine typing of isObject<T> #12259

merged 5 commits into from
Mar 9, 2023

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Mar 4, 2023

The template parameter for isObject is currently used to force cast the value passed to the function, although there is no guarantee nor check done to ensure that this is really the case.

This commit updates the typings so that the type passed to isObject is used to guide TypeScript during subsequent field type checking, assuming each field value is unknown and must also be type checked.

Example:

interface MyType {
    foo: string
}

const unknownValue: unknown = { foo: 2 };

if (isObject<MyType>(unknownValue)) {
    // We enter this branch because `unknownValue` is indeed an object,
    // and TypeScript now knows we might want to access the `foo` field
    // from our interface. But we can't yet assume to know the type of
    // `foo`, so it is still typed as `unknown` and we must check:
    if (isString(unknownValue.foo)) {
        // We won't get in this branch. But if we did, then `foo` is
        // now properly typed (and checked) as `string`.
    }
}

Fix #12253
Fix #12250

Close #12251

How to test

Should compile.

Review checklist

Reminder for reviewers

The template parameter for `isObject` is currently used to force cast
the value passed to the function, although there is no guarantee nor
check done to ensure that this is really the case.

This commit updates the typings so that the type passed to `isObject` is
used to guide TypeScript during subsequent field type checking,
assuming each field value is `unknown` and must also be type checked.

Example:

```ts
interface MyType {
    foo: string
}

const unknownValue: unknown = { foo: 2 };

if (isObject<MyType>(unknownValue)) {
    // We enter this branch because `unknownValue` is indeed an object,
    // and TypeScript now knows we might want to access the `foo` field
    // from our interface. But we can't yet assume to know the type of
    // `foo`, so it is still typed as `unknown` and we must check:
    if (isString(unknownValue.foo)) {
        // We won't get in this branch. But if we did, then `foo` is
        // now properly typed (and checked) as `string`.
    }
}
```
@paul-marechal paul-marechal requested review from msujew and tsmaeder March 4, 2023 05:39
@eneufeld
Copy link
Contributor

eneufeld commented Mar 6, 2023

@paul-marechal ,
I think this makes #12251 obsolete, do you agree?

packages/core/src/common/types.ts Outdated Show resolved Hide resolved
packages/core/src/common/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked that we're finding the bug @eneufeld detected with this change.

packages/markers/src/common/marker.ts Show resolved Hide resolved
packages/plugin-ext/src/plugin/types-impl.ts Outdated Show resolved Hide resolved
@paul-marechal
Copy link
Member Author

paul-marechal commented Mar 6, 2023

I think this makes #12251 obsolete, do you agree?

Yes and no. While this PR might fix the issue you found, I still think it would be for the best to update the ProblemMarker.is function such as:

export namespace ProblemMarker {
    export function is(node: unknown): node is ProblemMarker {
        return Marker.is(node) && node.kind === PROBLEM_KIND;
    }
}

Just for good measure. edit: I pushed a commit that does that.

@paul-marechal paul-marechal requested a review from tsmaeder March 8, 2023 14:29
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the changes 👍

@tsmaeder tsmaeder removed their request for review March 9, 2023 13:49
@paul-marechal paul-marechal merged commit 610eb13 into master Mar 9, 2023
@paul-marechal paul-marechal deleted the mp/isObject branch March 9, 2023 16:56
@github-actions github-actions bot added this to the 1.36.0 milestone Mar 9, 2023
@tsmaeder tsmaeder mentioned this pull request Mar 14, 2023
2 tasks
tsmaeder pushed a commit that referenced this pull request Mar 20, 2023
The template parameter for `isObject` is currently used to force cast
the value passed to the function, although there is no guarantee nor
check done to ensure that this is really the case.

This commit updates the typings so that the type passed to `isObject` is
used to guide TypeScript during subsequent field type checking,
assuming each field value is `unknown` and must also be type checked.

Example:

```ts
interface MyType {
    foo: string
}

const unknownValue: unknown = { foo: 2 };

if (isObject<MyType>(unknownValue)) {
    // We enter this branch because `unknownValue` is indeed an object,
    // and TypeScript now knows we might want to access the `foo` field
    // from our interface. But we can't yet assume to know the type of
    // `foo`, so it is still typed as `unknown` and we must check:
    if (isString(unknownValue.foo)) {
        // We won't get in this branch. But if we did, then `foo` is
        // now properly typed (and checked) as `string`.
    }
}
```
@vince-fugnitto vince-fugnitto added the quality issues related to code and application quality label Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality issues related to code and application quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isObject() asserts untrue types ProblemsView context menu only shows 'Collapse All'
5 participants