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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
89527ec
Correct type of ATN cache byptassAltsAtnCache
Mar 6, 2017
16cf602
Workaround odd error message from TSC 2.2.1
Mar 6, 2017
45a02dc
Workaround due to Microsoft/Typescript#14476
Mar 6, 2017
2ca78cf
Plus code-review feedback from Sam
Mar 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export abstract class Parser extends Recognizer<Token, ParserATNSimulator> {
*
* @see ATNDeserializationOptions.isGenerateRuleBypassTransitions
*/
private static readonly bypassAltsAtnCache = new WeakMap<string, ATN>();
private static readonly bypassAltsAtnCache = new Map<string, ATN>();

/**
* The error handling strategy for the parser. The default value is a new
Expand Down Expand Up @@ -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 •
edited
Loading

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.

Filed microsoft/TypeScript#14487

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.

}
return stack;
}
Expand Down
4 changes: 2 additions & 2 deletions src/dfa/DFAState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

return result;
}
else {
let removed = map.get(-1);
map.delete(-1);
map.set(PredictionContext.EMPTY_FULL_STATE_KEY, removed);
map.set(PredictionContext.EMPTY_FULL_STATE_KEY, removed as DFAState);
}
}

Expand Down