Assignability should check an index signature or rest type of the source against optional properties of the target. #27591

mattmccutchen · 2018-10-07T06:52:19Z

Fix one good break in the compiler itself.

against optional properties of the target. Fix one good break in the compiler itself. Fixes microsoft#27144.

RyanCavanaugh · 2018-10-09T19:33:29Z

typescript-bot · 2018-10-09T19:33:32Z

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 3b5caa3. You can monitor the build here. It should now contribute to this PR's status checks.

RyanCavanaugh · 2019-01-24T21:43:42Z

@typescript-bot test this

typescript-bot · 2019-01-24T21:43:44Z

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 3b5caa3. You can monitor the build here. It should now contribute to this PR's status checks.

RyanCavanaugh · 2019-01-24T22:28:18Z

This turned up some very strange changes in the RWC baseline that need reduction so that we can understand why they're happening and whether or not they should

src/compiler/checker.ts

sandersn · 2019-01-30T23:52:35Z

Here's a problem found in firebase-firestore:

interface Opt {
    a?: string
    b?: number
}
interface Dunk {
    [s: string]: {}
}
function f(o: Opt, d: Dunk) {
    d = o
}

Opt should be assignable to Dunk, but with the current PR, it's not.

sandersn · 2019-01-31T22:24:09Z

In the ancient snapshot of VS Code in the RWC, a cast is no longer allowed when the target has protected properties, even when there is some shared property. Here's a smaller repro:

interface UView {
	size: number;
	sizing: unknown;
	fixedSize: number;
	minimumSize: number;
	maximumSize: number;
	render(container: unknown, orientation: unknown): void;
	layout(size: number, orientation: unknown): void;
	focus(): void;
        extraExtraExtra: string;
}
abstract class Vue {

	size: number;
	protected _sizing: unknown;
	protected _fixedSize: number;
	protected _minimumSize: number;

	get sizing(): unknown { return this._sizing; }
	get fixedSize(): number { return this._fixedSize; }
	get minimumSize(): number { return this._minimumSize; }
	get maximumSize(): number { return Number.POSITIVE_INFINITY; }

	protected setFlexible(size?: number): void {
	}

	protected setFixed(size?: number): void {
	}

	abstract render(container: unknown, orientation: unknown): void;
	abstract focus(): void;
	abstract layout(size: number, orientation: unknown): void;
}
function ohno() {
    let uv: UView;
    let i2: Vue = <Vue>uv; // error here
}

And here's the smallest repro I could get:

interface Source {
    shared: unknown;

    extraExtraExtra: string;
}
abstract class Target {
    get shared(): unknown { return {}; }

    protected _secret: unknown;
}
function ohno() {
    let uv: Source;
    let i2: Target = <Target>uv; // error here
}

sandersn

Need to address repros found from RWC failures.

microsoft#27591 (comment)

mattmccutchen · 2019-02-10T20:23:25Z

Here's a problem found in firebase-firestore: [...]

Opt should be assignable to Dunk, but with the current PR, it's not.

That has nothing to do with this PR: I get the same error with TypeScript 3.3.3, which is expected because the target has an index signature and the source doesn't. If you meant "Dunk should be assignable to Opt, but with the current PR, it's not", then yes, that is precisely the change being made in the current PR, and we'd need to see more context to evaluate whether it's reasonable to require that code to be changed to satisfy the stricter checking in this PR.

In the ancient snapshot of VS Code in the RWC, a cast is no longer allowed when the target has protected properties, even when there is some shared property.

With both examples, I get the same error with TypeScript 3.3.3 as with this PR, which is expected: a cast requires that one type be comparable to the other.

RyanCavanaugh · 2020-01-08T21:35:50Z

@sandersn Please update this and re-run RWC so we can discuss in a week or two at a design meeting

sandersn · 2020-03-05T21:44:19Z

@ahejlsberg this might be related to your work on assignability of optionals with respect to intersections.

sandersn · 2020-03-06T17:23:06Z

@typescript-bot user test this
@typescript-bot test this

typescript-bot · 2020-03-06T17:23:10Z

Heya @sandersn, I've started to run the parallelized community code test suite on this PR at 4534335. You can monitor the build here.

typescript-bot · 2020-03-06T17:23:10Z

Heya @sandersn, I've started to run the extended test suite on this PR at 4534335. You can monitor the build here.

typescript-bot · 2020-03-06T17:54:17Z

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

sandersn · 2020-03-06T18:52:42Z

Failures look about the same in RWC, although I've not had a chance to check in detail. The user/docker tests have a lot more and more up-to-date code than when this PR was opened, so I'll look at that first. Plus the diff is a public PR.

There are no breaks from tuple rest types, but this isn't surprising since those are quite rare.

azure-sdk has two breaks, 1 unique.
office-ui-fabric has 34 failures, but only 5 unique.
vscode has 4 breaks, 2 unique. Both in electron-browser.

Iniitially, I guess that the breaks are correct. I'll have to actually look at the code, which means cloning and setting it up on my laptop...

weswigham · 2020-03-06T19:53:37Z

@typescript-bot pack this

typescript-bot · 2020-03-06T19:53:45Z

Heya @weswigham, I've started to run the tarball bundle task on this PR at 4534335. You can monitor the build here.

weswigham · 2020-03-06T20:32:31Z

The test failures in this PR need to be addressed before a tarball can be produced~

sandersn · 2020-03-06T22:27:36Z

Looking closer at VSCode, there's an example like this:

export interface ITelemetryData {
    from?: string;
    target?: string;
    [key: string]: any;
}
export type Tags = { [index: string]: boolean | number | string | undefined };
let d: ITelemetryData = tags

This has the incorrect error that

[XX:XX:XX] Error: /vscode/src/vs/workbench/contrib/tags/electron-browser/workspaceTags.ts(215,52): Argument of type 'Tags' is not assignable to parameter of type 'ITelemetryData'.
  Index signature is incompatible with property 'from'.
    Type 'string | number | boolean | undefined' is not assignable to type 'string | undefined'.

Which is true, but the assignment is still fine because ITelemetryData has an index signature to any. So I think the checks should not apply when the target has an index signature.

sandersn · 2020-03-06T22:54:40Z

The other VSCode error is not catching a bug either, although it's closer:

(['includeSystemInfo', 'includeProcessInfo', 'includeWorkspaceInfo', 'includeExtensions', 'includeSearchedExtensions', 'includeSettingsSearchDetails'] as const).forEach(elementId => {
                var x = { [elementId]: !this.issueReporterModel.getData()[elementId] } as unknown as Partial<IssueReporterData>
                let pird: Partial<IssueReporterData> = x

where

export interface IssueReporterData {
	issueType: IssueType;
	issueDescription?: string;

	versionInfo?: any;
	systemInfo?: SystemInfo;
	processInfo?: any;
	workspaceInfo?: any;

	includeSystemInfo: boolean;
	includeWorkspaceInfo: boolean;
	includeProcessInfo: boolean;
	includeExtensions: boolean;
	includeSearchedExtensions: boolean;
	includeSettingsSearchDetails: boolean;

	numberOfThemeExtesions?: number;
	allExtensions: IssueReporterExtensionData[];
	enabledNonThemeExtesions?: IssueReporterExtensionData[];
	extensionsDisabled?: boolean;
	fileOnExtension?: boolean;
	selectedExtension?: IssueReporterExtensionData;
	actualSearchResults?: ISettingSearchResult[];
	query?: string;
	filterResultCount?: number;
}

In this case, the intent is sound, but we give x: { [s: string]: boolean } instead of x: { includeSystemInfo?: boolean, includeWorkspaceInfo?: boolean, ... }. Then that index signature is not assignment to IssueReporterData.

sandersn · 2020-03-06T23:55:06Z

I pushed a commit to not handle private identifiers. That might not be the right thing, since I think private identifiers can be optional, but I'm just trying to get the tests to pass for now.

DanielRosenwasser · 2020-03-09T18:03:26Z

That might not be the right thing, since I think private identifiers can be optional,

An index signature simply doesn't need to apply to a private field because there's no way to dynamically look it up through an element access of the form expr[expr].

DanielRosenwasser · 2020-03-09T18:12:01Z

(though it's not totally off limits in future ECMAScript)

sandersn · 2020-03-13T17:30:22Z

An API test breaks, finding that CompilerOptionsValue is not assignable to WatchFileKind:

    type CompilerOptions = number | ...
    export enum WatchFileKind {
        FixedPollingInterval = 0,
        PriorityPollingInterval = 1,
        DynamicPriorityPolling = 2,
        UseFsEvents = 3,
        UseFsEventsOnParentDirectory = 4
    }

This is a very subtle bug because it is buried so deep, and it goes through a parameter in a signature so the variance flips, making it extra confusing. But it is correct.
I just skipped it with a cast to any for now; if this sample is trying to show 'correct' usage of our API, it's showing that API is error-prone!

sandersn · 2020-03-13T18:30:51Z

The azure-sdk error is probably good, easy to understand, and in fact got fixed 4 days ago in 87e59fd300. It seems like the predicate used to distinguish X | { [s: string]: X } was insufficient, so one code path was incorrectly passing a { [s: string]: X } to a function that expects X.

(It's possible that the predicate was fine, and the compiler just couldn't prove it.)

sandersn · 2020-03-13T18:56:47Z

Welp, now the LKG build catches a failures in the server/editorServices.ts, in which WatchOptions is not assignable to ExternalProjectCompilerOptions. It's a correct error: WatchOptions has an index signature that includes a whole bunch of types, but ExternalProjectCompilerOptions has many optional properties of type boolean | undefined.

This is sound but I've been staring at this too long; I can't tell if it's desirable or not.

sheetalkamat · 2020-03-13T20:24:10Z

tests/baselines/reference/APISample_WatchWithOwnWatchHost.js

@@ -36,8 +36,8 @@ function watchMain() {
        readDirectory: ts.sys.readDirectory,
        realpath: ts.sys.realpath,

-        watchFile: ts.sys.watchFile!,
-        watchDirectory: ts.sys.watchDirectory!,
+        watchFile: ts.sys.watchFile! as any,


ts.sys.watchFile is a function with a final parameter of type WatchOptions, which has a property of type WatchFileKind, a numeric enum. CompilerHost's watchFile, the target signature, has a final parameter of type CompilerOptions, which does not have a matching property, but does have an index signature that includes number. But number isn't assignable to an enum.

This is technically sound, because people could call watchFile with an options bug containing any number at all (or a lot of other types), but ts.sys.watchFile can only handle WatchFileKind on certain of its properties.

However, I don't know how likely this error is to happen in practise. We haven't had trouble as far as we know.

sandersn

From my observation, this change is:

Technically correct in most (probably all?) cases.
But not practically useful in the majority of cases.
Hard to recover from in many cases.

I vote not to take this change unless I see something that changes my mind. For example:

Data that show a majority of good, easy-to-understand bugs being caught.
An easy explanation of how to fix the errors mechanically.

@DanielRosenwasser I'd like a second opinion if you have time.

sandersn · 2020-03-13T21:09:44Z

@typescript-bot user test this

typescript-bot · 2020-03-13T21:09:49Z

Heya @sandersn, I've started to run the parallelized community code test suite on this PR at 133968c. You can monitor the build here.

typescript-bot · 2020-03-13T21:40:30Z

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

weswigham · 2020-03-13T21:56:03Z

@typescript-bot pack this now that it's fixed up

typescript-bot · 2020-03-13T21:56:04Z

Heya @weswigham, I've started to run the tarball bundle task on this PR at 133968c. You can monitor the build here.

sandersn · 2020-04-01T22:33:17Z

This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here.

Assignability should check an index signature or rest type of the source

3b5caa3

against optional properties of the target. Fix one good break in the compiler itself. Fixes microsoft#27144.

mattmccutchen mentioned this pull request Oct 7, 2018

Index signature is assignable to weak type whose properties don't match the signature type #27144

Open

RyanCavanaugh assigned sandersn Jan 24, 2019

sandersn added 2 commits January 29, 2019 09:24

Merge branch 'master' into issue-27144

b3a22ac

Merge branch 'master' into issue-27144

20d7b20

sandersn reviewed Jan 30, 2019

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

src/compiler/checker.ts Outdated Show resolved Hide resolved

Merge branch 'master' into issue-27144

e4e34f8

sandersn requested changes Feb 1, 2019

View reviewed changes

mattmccutchen added 2 commits February 10, 2019 15:02

Merge remote-tracking branch 'upstream/master' into issue-27144

f889cc5

Style change

bdfb193

microsoft#27591 (comment)

sandersn added the For Backlog Bug PRs that fix a backlog bug label Feb 1, 2020

Merge branch 'master' into issue-27144

4534335

typescript-bot mentioned this pull request Mar 6, 2020

🤖 User test baselines have changed for issue-27144 mattmccutchen/TypeScript#1

Open

do not handle private identifiers

afac48c

chradek mentioned this pull request Mar 9, 2020

[event-hubs] better type guards for event position Azure/azure-sdk-for-js#7718

Merged

sandersn added 2 commits March 13, 2020 09:37

check for undefined valueDeclaratio too

5fa6dfd

quick fix for API test failure

e428ed6

cover up another error

133968c

sheetalkamat reviewed Mar 13, 2020

View reviewed changes

sandersn requested changes Mar 13, 2020

View reviewed changes

typescript-bot mentioned this pull request Mar 13, 2020

🤖 User test baselines have changed for issue-27144 mattmccutchen/TypeScript#2

Open

DanielRosenwasser mentioned this pull request Mar 13, 2020

Design Meeting Notes, 3/6/2020 #37388

Closed

sandersn closed this Apr 1, 2020

Assignability should check an index signature or rest type of the source against optional properties of the target. #27591

Assignability should check an index signature or rest type of the source against optional properties of the target. #27591

Conversation

mattmccutchen commented Oct 7, 2018

RyanCavanaugh commented Oct 9, 2018

typescript-bot commented Oct 9, 2018 • edited Loading

RyanCavanaugh commented Jan 24, 2019

typescript-bot commented Jan 24, 2019 • edited Loading

RyanCavanaugh commented Jan 24, 2019

sandersn commented Jan 30, 2019

sandersn commented Jan 31, 2019

sandersn left a comment

Choose a reason for hiding this comment

mattmccutchen commented Feb 10, 2019

RyanCavanaugh commented Jan 8, 2020

sandersn commented Mar 5, 2020

sandersn commented Mar 6, 2020

typescript-bot commented Mar 6, 2020 • edited Loading

typescript-bot commented Mar 6, 2020 • edited Loading

typescript-bot commented Mar 6, 2020

sandersn commented Mar 6, 2020

weswigham commented Mar 6, 2020

typescript-bot commented Mar 6, 2020 • edited Loading

weswigham commented Mar 6, 2020

sandersn commented Mar 6, 2020

sandersn commented Mar 6, 2020

sandersn commented Mar 6, 2020

DanielRosenwasser commented Mar 9, 2020

DanielRosenwasser commented Mar 9, 2020

sandersn commented Mar 13, 2020

sandersn commented Mar 13, 2020

sandersn commented Mar 13, 2020

sheetalkamat Mar 13, 2020

Choose a reason for hiding this comment

sandersn Mar 13, 2020

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

sandersn commented Mar 13, 2020

typescript-bot commented Mar 13, 2020 • edited Loading

typescript-bot commented Mar 13, 2020

weswigham commented Mar 13, 2020

typescript-bot commented Mar 13, 2020 • edited Loading

sandersn commented Apr 1, 2020

typescript-bot commented Oct 9, 2018 •

edited

Loading

typescript-bot commented Jan 24, 2019 •

edited

Loading

typescript-bot commented Mar 6, 2020 •

edited

Loading

typescript-bot commented Mar 6, 2020 •

edited

Loading

typescript-bot commented Mar 6, 2020 •

edited

Loading

typescript-bot commented Mar 13, 2020 •

edited

Loading

typescript-bot commented Mar 13, 2020 •

edited

Loading