Skip to content

3.9 regression: bogus property-will-be-overwritten-by-spread errors #36779

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

Closed
sandersn opened this issue Feb 13, 2020 · 11 comments · Fixed by #37192
Closed

3.9 regression: bogus property-will-be-overwritten-by-spread errors #36779

sandersn opened this issue Feb 13, 2020 · 11 comments · Fixed by #37192
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@sandersn
Copy link
Member

export class Terror extends Error {
    public get telemetry() {
        return { command: "bye", ok: "next" }
    }
}
function g(u: unknown, b: boolean, t: Terror) {
    return { command: "hi", ...(b ? t.telemetry : {}) }
}

Expected behavior:

No error; command is not necessarily overwritten, for example when b=false.

Actual behavior:

Error: 'command' is specified more than once, so this usage will be overwritten.

User tests vscode and office-fabric-ui have failures because of this.

@DanielRosenwasser
Copy link
Member

To clarify, this isn't a 3.8 RC issue, right?

@sandersn
Copy link
Member Author

sandersn commented Feb 13, 2020

Yep, that's why I put '3.9' in the title.

(The code is only in master, not in 3.8, since I merged it after the release branch got merged from master)

@sandersn sandersn self-assigned this Feb 13, 2020
@sandersn sandersn added this to the TypeScript 3.9.0 milestone Feb 13, 2020
@sandersn sandersn added the Bug A bug in TypeScript label Feb 13, 2020
@JakeGinnivan
Copy link

JakeGinnivan commented Mar 1, 2020

Our code base got hit by this as well when trying out the latest @next build. Mainly in options objects and css in JS when you provide a default and spread an object which can optionally specify a key.

sandersn added a commit that referenced this issue Mar 3, 2020
Trying to do this check in getSpreadType just doesn't have enough
information, so I moved it to checkObjectLiteral, which is a better
place for issuing errors anyway.

Unfortunately, the approach is kind of expensive in that it

1. creates a new map for each property and
2. iterates over all properties of the spread type, even if it's a
union.

I have some ideas to improve (1) that might work out. I'm not sure how
bad (2) is since we're going to iterate over all properties of all
constituents of a union.

Fixes #36779
@sandersn sandersn added the Fix Available A PR has been opened for this issue label Mar 3, 2020
sandersn added a commit that referenced this issue Mar 3, 2020
* More precise property-overwritten-by-spread errors

Trying to do this check in getSpreadType just doesn't have enough
information, so I moved it to checkObjectLiteral, which is a better
place for issuing errors anyway.

Unfortunately, the approach is kind of expensive in that it

1. creates a new map for each property and
2. iterates over all properties of the spread type, even if it's a
union.

I have some ideas to improve (1) that might work out. I'm not sure how
bad (2) is since we're going to iterate over all properties of all
constituents of a union.

Fixes #36779

* another test and rename
@mvestergaard
Copy link

mvestergaard commented Mar 29, 2020

I think this might still be an issue in JSX?
I've tried upgrading our codebase to both @beta and @next and they both give me these kinds of errors in any JSX that specifies a prop which could be overwritten by a spread:

src/features/search/components/SearchResult/SearchResult.tsx:89:92 - error TS2783: 'alignItems' is specified more than once, so this usage will be overwritten.

89   <Box flex="0 0 auto" display="flex" flexDirection="column" justifyContent="space-evenly" alignItems="flex-end" {...rest}>
                                                                                              ~~~~~~~~~~~~~~~~~~~~~

@janicduplessis
Copy link

I'm also getting a few of those since updating to 3.9-beta. Seems related to any type usage.

type Props = {
  someProp?: any;
};
function Component(props: Props) {
  return <View {...props} someProp={{}} />
}

Using another type instead of any, for example object does not trigger the error.

@bradleyayers
Copy link

bradleyayers commented Mar 31, 2020

@janicduplessis I'm not sure if this will be on their radar, given this issue is closed. Perhaps open a new issue? FWIW I find that unknown has the same behaviour as any.

@sandersn
Copy link
Member Author

sandersn commented Apr 1, 2020

@janicduplessis please do; I tried a simple repro like yours but couldn't get it to work since I don't have View declared.

I just merged a fix from a community member that might help, but it didn't make the beta. Can you try typescript@next before filing the bug?

@mvestergaard
Copy link

mvestergaard commented Apr 1, 2020

@sandersn Here's a small reproduction repo https://github.com/mvestergaard/ts-36779-repro using 3.9.0-dev.20200330

@janicduplessis
Copy link

janicduplessis commented Apr 1, 2020

@sandersn Thanks for looking into it, I could have made a better repro. See #37740

@TheJoeSchr
Copy link

I also have this, but I don't see store getting overwritten...

    const store = provideClearingStore(ctx, nuxtInject);
    const apolloClient = provideVueApolloComposable(ctx);
    (ctx as any).store = store; // delete vuex
    provideGlobalsForCypress({ store, apolloClient, ...ctx });
    return { store, apolloClient };

Error:

 13:32 'store' is specified more than once, so this usage will be overwritten.
     11 |     const apolloClient = provideVueApolloComposable(ctx);
     12 |     (ctx as any).store = store; // del files.ete vuex
   > 13 |     provideGlobalsForCypress({ store,apolloClient, ...ctx });
        |                                ^
     14 |     return { store, apolloClient };   provide-globals-once.ts(13,32):
     15 |   };                                  his usage will be overwritten.
     16 | };   

@TheJoeSchr
Copy link

AH, I get it, it's also in my ctx which also get spread in and since I casted it to any before, TS doesn't get it. Sorry for the noise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants