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

TS 2.4.1 flags legal code as invalid? #17000

Closed
dbaeumer opened this issue Jul 7, 2017 · 6 comments
Closed

TS 2.4.1 flags legal code as invalid? #17000

dbaeumer opened this issue Jul 7, 2017 · 6 comments
Labels
Duplicate An existing issue was already created

Comments

@dbaeumer
Copy link
Member

dbaeumer commented Jul 7, 2017

TypeScript Version: 2.4.1

Code

namespace Config {
	export interface ProblemPattern {
		regexp?: string;
		file?: number;
		location?: number;
		line?: number;
		column?: number;
		endLine?: number;
		endColumn?: number;
		severity?: number;
		code?: number;
		message?: number;
		loop?: boolean;
	}

	export interface NamedProblemPattern extends ProblemPattern {
		name: string;
		label?: string;
	}

	export type MultiLineProblemPattern = ProblemPattern[];

	export interface NamedMultiLineProblemPattern {
		name: string;
		label?: string;
		patterns: MultiLineProblemPattern;
	}
}

export interface ProblemPattern {
	regexp: RegExp;
	file?: number;
	message?: number;
	location?: number;
	line?: number;
	character?: number;
	endLine?: number;
	endCharacter?: number;
	code?: number;
	severity?: number;
	loop?: boolean;
}

export interface NamedProblemPattern extends ProblemPattern {
	name: string;
}

export type MultiLineProblemPattern = ProblemPattern[];

export interface NamedMultiLineProblemPattern {
	name: string;
	label: string;
	patterns: MultiLineProblemPattern;
}

class Parser {

	constructor() {}

	public parse(value: Config.ProblemPattern): ProblemPattern;
	public parse(value: Config.MultiLineProblemPattern): MultiLineProblemPattern[];
	public parse(value: Config.NamedProblemPattern): NamedProblemPattern;
	public parse(value: Config.NamedMultiLineProblemPattern): NamedMultiLineProblemPattern;
	public parse(value: Config.ProblemPattern | Config.MultiLineProblemPattern | Config.NamedProblemPattern | Config.NamedMultiLineProblemPattern): any {
	}
}

function foo(value: string | Config.ProblemPattern | Config.MultiLineProblemPattern): void {
	if (typeof value === 'string') {
		//
	} else {
		let parser = new Parser();
		parser.parse(value);
	}
}

Expected behavior:
No error message

Actual behavior:
Flags an error on line 73

@kitsonk
Copy link
Contributor

kitsonk commented Jul 7, 2017

It isn't technically valid code. At this point value is 'ProblemPattern | ProblemPattern[]', which does not match any patterns of the overload. Because ProblemPattern was a weak type, it was essentially equated to any and therefore assignable to things it shouldn't have been allowed to be assigned to (would essentially be any | any[] which becomes any. Now, with weak type detection it is properly erroring. .parse() either needs to accept an array of ProblemPattern[] or it needs to be handled before calling parse.

@ghost
Copy link

ghost commented Jul 7, 2017

@kitsonk There is an overload for each of those types, but AFAICT the parser wants an overload that accepts a union of both types. Which makes sense, because the two overloads return different types, and so it's ambiguous as to what the return type should be.

I've previously argued that overloads with ambiguous return values should return a union of their return types, but the TS team did not agree.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 7, 2017

There is an overload for each of those types

Which overload matches ProblemPattern | ProblemPattern[]? I can't see any where it accepts an array as an argument, but because prior to 2.4, ProblemPattern being a weak type would essentially be treated as any and so therefore ProblemPattern | ProblemPattern[] would equate to any | any[] which would equate to any and therefore, TypeScript was allowing it.

@ghost
Copy link

ghost commented Jul 7, 2017

public parse(value: Config.ProblemPattern): ProblemPattern; matches ProblemPattern
public parse(value: Config.MultiLineProblemPattern): MultiLineProblemPattern[]; matches ProblemPattern[]

No overload matches a union of both, which is the issue.

My argument in the linked issue was that those two overloads together imply an overload of
public parse(value: Config.ProblemPattern|Config.MultiLineProblemPattern): ProblemPattern|MultiLineProblemPattern[]; (but I didn't win that argument).

@kitsonk
Copy link
Contributor

kitsonk commented Jul 7, 2017

@errorx666 you are correct, because it was named, I wasn't doing the mental gymnastics... the reason it used to be valid was because of weak types but yes, now, yes... the other problem.

There is also this pattern:

public parse(value: Config.ProblemPattern | Config.MultiLineProblemPattern | Config.NamedProblemPattern | Config.NamedMultiLineProblemPattern): any;

Which would imply that Config.ProblemPattern | Config.MultiLineProblemPattern without the other two unions would be acceptable. If that isn't acceptable, I don't know what is... ;-) so I agree with you now!

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jul 7, 2017
@RyanCavanaugh
Copy link
Member

Duplicate #1805

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

3 participants