Skip to content

Fix errors reported under Typescript 2.2.1 #286

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

Merged
merged 4 commits into from
Mar 6, 2017
Merged

Fix errors reported under Typescript 2.2.1 #286

merged 4 commits into from
Mar 6, 2017

Conversation

BurtHarris
Copy link
Collaborator

@BurtHarris BurtHarris commented Mar 6, 2017

Workaround several minor error messages introduced when switching to the Typescript 2.2.1 transpiler. Fixes #285

I'm not sure why my name & sigil are showing up twice in the commits...

@BurtHarris BurtHarris changed the title Fix.ts.2.2 Fix errors reported under Typescript 2.2.1 Mar 6, 2017
@@ -170,13 +170,13 @@ export class DFAState {
if (map.has(-1)) {
if (map.size === 1) {
let result = new Map<number, DFAState>();
result.set(PredictionContext.EMPTY_FULL_STATE_KEY, map.get(-1));
result.set(PredictionContext.EMPTY_FULL_STATE_KEY, map.get(-1) as DFAState);
Copy link
Member

Choose a reason for hiding this comment

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

💡 The following equivalent change would not require the type assertion:

getContextEdgeMap(): Map<number, DFAState> {
	let map = new Map<number, DFAState>(this.contextEdges);
	let existing = map.get(-1);
	if (existing !== undefined) {
		if (map.size === 1) {
			let result = new Map<number, DFAState>();
			result.set(PredictionContext.EMPTY_FULL_STATE_KEY, existing);
			return result;
		}
		else {
			map.delete(-1);
			map.set(PredictionContext.EMPTY_FULL_STATE_KEY, existing);
		}
	}

	return map;
}

@@ -807,7 +807,7 @@ export abstract class Parser extends Recognizer<Token, ParserATNSimulator> {
let ruleIndex: number = p.ruleIndex;
if (ruleIndex < 0) stack.push("n/a");
else stack.push(ruleNames[ruleIndex]);
p = p._parent;
p = p._parent as RuleContext;
Copy link
Member

Choose a reason for hiding this comment

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

💭 Maybe we should check with the TypeScript project about what they expect this code to look like with TypeScript 2.2.

Copy link
Member

Choose a reason for hiding this comment

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

➡️ The issue is the use of a non-undefined default argument overrides the type annotation, so the compiler has assumed that p cannot be undefined. The solution here (a workaround assuming that this is a bug in the compiler) is assigning p to a local variable at the beginning of the method, and operating on the local variable instead of p itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I tried a local variable and that worked without problem too. I liked this version because it touched less lines of code.

I'm trying to put together an isolated repro case to file a bug with the TypeScript team.

Copy link
Member

@sharwell sharwell Mar 6, 2017

Choose a reason for hiding this comment

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

Yes, I tried a local variable and that worked without problem too. I liked this version because it touched less lines of code.

The form used here is shorter but incorrect. You assert that p._parent is a RuleContext (not undefined), but if that is always the case then the code is an infinite loop.

I'm trying to put together an isolated repro case to file a bug with the TypeScript team.

Here is a self-contained reproducer:

var obj: Context = {
  parent: undefined
};

function working(p: Context | undefined) {
  while (p != null) {
    p = p.parent;
  }
}

function broken(p: Context | undefined = obj) {
  while (p != null) {
    p = p.parent;
  }
}

interface Context { parent: Context | undefined; }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The Typescript dev lead's note in above bug suggests they will treat this as a bug. Updated code based on your review.

@sharwell sharwell added this to the 0.4.0-alpha.5 milestone Mar 6, 2017
@sharwell sharwell merged commit 744c1ea into tunnelvisionlabs:master Mar 6, 2017
@BurtHarris BurtHarris deleted the fix.ts.2.2 branch March 7, 2017 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants