DeclarationReference: Parser does not handle certain inputs correctly

[octogonz]

@rbuckton I'm using this issue to track some bugs found while testing the DeclarationReference API. They are minor enough to be fixed in the same PR.

1. The @microsoft/rush-stack-compiler-3.5 package name is not handled correctly because of the . in the name:

   // (Actual) SyntaxError: SyntaxError: Invalid Component '@microsoft/rush-stack-compiler-3.5'
   // (Expected) @microsoft/rush-stack-compiler-3.5!
   console.log(DeclarationReference.package('@microsoft/rush-stack-compiler-3.5').toString());
   
   // (Actual) @microsoft/rush-stack-compiler-3.5
   // (Expected) @microsoft/rush-stack-compiler-3.5!
   console.log(DeclarationReference.parse('@microsoft/rush-stack-compiler-3.5!').toString());
   
   // (Works correctly) @scope/package!
   console.log(DeclarationReference.parse('@scope/package!').toString());

2. The parser wrongly accepts an NPM package name that is missing the !. The input should really be interpreted as a package, because @ is not a valid identifier, but packages should require the ! suffix to avoid confusion:

   // (Actual) @scope/my-package
   // (Expected) SyntaxError
   console.log(DeclarationReference.parse('@scope/my-package').toString());
   
   // (Works correctly) @scope/my-package!
   console.log(DeclarationReference.parse('@scope/my-package!').toString());

3. The parser wrongly accepts identifiers containing spaces.

   // (Actual) a.b c.d
   // (Expected) SyntaxError
   console.log(DeclarationReference.parse('a.b c.d').toString());

[octogonz]

@rbuckton Seems like this is not an LR(1) problem. The current TSDoc parser scans forwards looking for the package delimiter (# instead of !) like this:

tsdoc/tsdoc/src/parser/NodeParser.ts

Lines 927 to 992 in 79c5511

	// The package name can contain characters that look like a member reference. This means we need to scan forwards
	// to see if there is a "#". However, we need to be careful not to match a "#" that is part of a quoted expression.
	const marker: number = tokenReader.createMarker();
	let hasHash: boolean = false;
	// A common mistake is to forget the "#" for package name or import path. The telltale sign
	// of this is mistake is that we see path-only characters such as "@" or "/" in the beginning
	// where this would be a syntax error for a member reference.
	let lookingForImportCharacters: boolean = true;
	let sawImportCharacters: boolean = false;
	let done: boolean = false;
	while (!done) {
	switch (tokenReader.peekTokenKind()) {
	case TokenKind.DoubleQuote:
	case TokenKind.EndOfInput:
	case TokenKind.LeftCurlyBracket:
	case TokenKind.LeftParenthesis:
	case TokenKind.LeftSquareBracket:
	case TokenKind.Newline:
	case TokenKind.Pipe:
	case TokenKind.RightCurlyBracket:
	case TokenKind.RightParenthesis:
	case TokenKind.RightSquareBracket:
	case TokenKind.SingleQuote:
	case TokenKind.Spacing:
	done = true;
	break;
	case TokenKind.PoundSymbol:
	hasHash = true;
	done = true;
	break;
	case TokenKind.Slash:
	case TokenKind.AtSign:
	if (lookingForImportCharacters) {
	sawImportCharacters = true;
	}
	tokenReader.readToken();
	break;
	case TokenKind.AsciiWord:
	case TokenKind.Period:
	case TokenKind.Hyphen:
	// It's a character that looks like part of a package name or import path,
	// so don't set lookingForImportCharacters = false
	tokenReader.readToken();
	break;
	default:
	// Once we reach something other than AsciiWord and Period, then the meaning of
	// slashes and at-signs is no longer obvious.
	lookingForImportCharacters = false;
	tokenReader.readToken();
	}
	}
	if (!hasHash && sawImportCharacters) {
	// We saw characters that will be a syntax error if interpreted as a member reference,
	// but would make sense as a package name or import path, but we did not find a "#"
	this._parserContext.log.addMessageForTokenSequence(
	TSDocMessageId.ReferenceMissingHash,
	'The declaration reference appears to contain a package name or import path,'
	+ ' but it is missing the "#" delimiter',
	tokenReader.extractAccumulatedSequence(), nodeForErrorContext);
	return undefined;
	}

Even if you could somehow represent this as LR(1), seems like the error messages may be better if the algorithm can tell whether something looks like an NPM package or not.

[rbuckton]

@octogonz @microsoft/rush-stack-compiler-3.5 may not work, but have you tried "@microsoft/rush-stack-compiler-3.5"? Since it includes a . it probably needs to be quoted. I can look at improving this when time allows.

[octogonz]

I might be able to work on it this weekend. Do you have any objection to simply scanning forwards until we find the ! delimiter and then parsing the two halves separately? That's how the old parser works. I don't know how to represent it in your grammar, though.

Quoted NPM package names doesn't seem like a good option, since package names and import paths never need escapes in practice. They are much better behaved than TypeScript identifiers (which e.g. may be forced to match a freeform string key in an interface for a JSON object).

[rbuckton]

Do you have any objection to simply scanning forwards until we find the ! delimiter and then parsing the two halves separately?

No, its probably not a problem. I took a look and should have a PR up shortly.

[rbuckton]

@octogonz: #177 contains the related PR.

[octogonz]

@rbuckton I validated that issues (1) and (2) are fixed, but issue (3) still repros:

// (Actual) a.b c.d
// (Expected) SyntaxError
console.log(DeclarationReference.parse('a.b c.d').toString());

I'm going to go ahead and merge your PR, though, so we can unblock microsoft/rushstack#1406. I'll open a separate issue for this (relatively minor) edge case.