Adds ':call', ':new', and ':index' and fixes a parsing bug. #173

rbuckton · 2019-07-22T03:07:57Z

Adds ':call', ':new', and ':index' per microsoft/rushstack#1406 (comment).

Fixes a parsing bug per microsoft/rushstack#1406 (comment).

octogonz · 2019-07-22T04:41:44Z

tsdoc/src/beta/DeclarationReference.grammarkdown

+  `new`                                         // SymbolFlags.Signature (for __new)
+  `index`                                       // SymbolFlags.Signature (for __index)
+  `signature`                                   // SymbolFlags.Signature (deprecated)
  `type`                                        // Any complex type


@rbuckton In microsoft/rushstack#1406 (comment) I am proposing the following list of meanings:

call

class

constructor

enum

function

imember (covers MethodSignature and PropertySignature)

index

interface

member (covers EnumMember, Method, and Property)

namespace

new

type (replaces typealias because I believe it's more intuitive to use TypeScript keywords than compiler internals)

var

What do you think?

imember - unnecessary, member works fine for these.

var - I suppose its shorter than variable

type - There is already a type and it has a different purpose than typealias. "type" is also a very general term, while a "type alias" is what a type keyword produces. I disagree that we should use the keyword over the semantic meaning here.

imember - unnecessary, member works fine for these.

This seems like a counterexample:

interface X { y: number; // X#y:member } class X { public y: number; // X#y:member }

How would you handle it?

As far as TypeScript is concerned, they are the same member. If you try to change the type of one to "string", you get a "duplicate identifier" error because they collide.

case in point:

interface X { y: number; } interface X { y: number; // change to `string` to get the same "duplicate identifier" error }

Here's a counterproposal:

call for SyntaxKind.Signature with name __call

new for SyntaxKind.Signature with name __new

index for SyntaxKind.Signature with name __index

variable becomes var

type becomes complex

typealias becomes type

enummember becomes member

member satisfies the following meanings:

Method

MethodSignature

Property

PropertySignature

GetAccessor

SetAccessor

EnumMember

There is a possible conflict in meanings for member, but it seems trivial as TypeScript still treats them as the same member:

class C { get x(): number; } interface C { x: number; }

Thinking about it more, the class+interface merge is fairly bizarre. Nobody can implement the interface without also implementing the class members. It's a bit of a puzzle how the documentation system should even represent that. I agree that imember won't be needed or meaningful in practice.

👍 I'm good with this proposal.

octogonz · 2019-07-22T04:42:23Z

tsdoc/src/beta/DeclarationReference.grammarkdown

+  `call`                                        // SymbolFlags.Signature (for __call)
+  `new`                                         // SymbolFlags.Signature (for __new)
+  `index`                                       // SymbolFlags.Signature (for __index)
+  `signature`                                   // SymbolFlags.Signature (deprecated)


We can simply delete signature. This API is "beta".

octogonz · 2019-07-22T04:57:18Z

tsdoc/src/beta/DeclarationReference.ts

    return reference;
  }

  public static makeSafeComponent(text: string): string {


@rbuckton in microsoft/rushstack#1406 (comment) I am asking that .addNavigationStep() should treat its input as an unquoted identifier, rather than parsing it.

Current design:

.addNavigationStep("[X]") --> [X] (i.e. parsed as an ECMAScript symbol)

.addNavigationStep("[X") --> "[X" (i.e. a quoted identifier)

.addNavigationStep("\"X\"") --> "X"

.addNavigationStep("X") --> X

What I think is a better design:

.addNavigationStep("[X]") --> "[X]" (still an identifier, not reinterpreted as an ECMAScript symbol expression)

.addNavigationStep("[X") --> "[X"

.addNavigationStep("\"X\"") --> X (quotes removed because they are redundant)

.addNavigationStep("X") --> X

My proposal should also eliminate the unexceptional exception (catch { // do nothing }).

We can probably use StringChecks.explainIfInvalidUnquotedIdentifier() to determine whether quotes are needed.

.addNavigationStep("[X]") --> "[X]"`

I completely disagree. One of the things I wanted to fix with all of this was having proper names for computed property names, as I have a large number of APIs that I am using api-extractor/api-documenter for that use unique symbol types, and I fully intended for this to address those use cases as well.

.addNavigationStep("\"X\"") --> X`

I also disagree with this. It should be what was written, or encoded if what was written isn't a valid component.

Consider a URL: If I create a URL from "http://foo.com/bar%2520baz" and decode it, then I end up with "http://foo.com/bar%20baz", which could easily be misinterpreted as "http://foo.com/bar baz" if you pass it to another API. However, if I create a URL from "http://foo.com/bar baz" it must be encoded as "http://foo.com/bar%20baz" because the space character is not permitted.

I think we should reserve quotes by default, so a quoted string containing a quoted string should be escaped:

.addNavigationStep("[X]") -> [X]

.addNavigationStep("[X") -> "[X"

.addNavigationStep("\"X\"") -> "\"X\""

.addNavigationStep("X") -> X

If we need to pass through quotes, we should add an optional argument to addNavigationStep to indicate the string is already escaped (in which case we should only validate it).

Ahh... I did not realize that in your DeclarationReference API, .addNavigationStep("[X]") is the only way to make an ECMAScript symbol. I was confused because in the TSDoc parser, the AST and API have special nodes for representing an ECMAScript symbol and its internal components.

For example, this:

/** {@link my-control-library#Button.[UISymbols.toNumberPrimitive]} */

...gets parsed as this (in the TSDoc playground):

- LinkTag * InlineTag_OpeningDelimiter="{" * InlineTag_TagName="@link" * Spacing=" " - DeclarationReference * DeclarationReference_PackageName="my-control-library" * DeclarationReference_ImportHash="#" - MemberReference - MemberIdentifier * MemberIdentifier_Identifier="Button" - MemberReference * MemberReference_Dot="." - MemberSymbol * DocMemberSymbol_LeftBracket="[" - DeclarationReference - MemberReference - MemberIdentifier * MemberIdentifier_Identifier="UISymbols" - MemberReference * MemberReference_Dot="." - MemberIdentifier * MemberIdentifier_Identifier="toNumberPrimitive" * DocMemberSymbol_RightBracket="]" * InlineTag_ClosingDelimiter="}"

The UISymbols.toNumberPrimitive substring becomes a second declaration reference nested inside the outer one, and that is what TSDoc's DocMemberSymbol constructor API receives.

For a "beta" prototype, I'm fine with your current approach of representing a symbol as a string, but we would need to fix it longer term. Otherwise it may be awkward for the caller to construct an input like [a.[b]."[c]"]. (The TypeScript compiler may provide utilities that help build expressions like this, but strictly speaking, it's making TypeScript expressions not UID expressions; there may be edge cases where the escaping rules differ. Ideally the TypeScript AST need to be transformed into TSDoc AST.)

I completely disagree. One of the things I wanted to fix with all of this was having proper names for computed property names, as I have a large number of APIs that I am using api-extractor/api-documenter for that use unique symbol types, and I fully intended for this to address those use cases as well.

I agree that ECMAScript symbols should be handled better by API Extractor. But the current implementation is somewhat of a hack. Here's how the name .api.json field ends up for some various inputs:

export declare const c: unique symbol; export declare class X { "a": string; // "name": "a" "b[": string; // "name": "b[" [c]: string; // "name": "[c]" d: string; // "name": "d" "[e]": string; // "name": "[e]" }

The reason is that this code calls tryGetLateBoundName() only when a symbol is encountered, which is technically mixing escaping contexts.

If we pass this through to addNavigationStep() directly then "[e]" will get misinterpreted.

I think we should reserve quotes by default, so a quoted string containing a quoted string should be escaped:

.addNavigationStep("[X]") -> [X]

.addNavigationStep("[X") -> "[X"

.addNavigationStep("\"X\"") -> "\"X\""

.addNavigationStep("X") -> X

If we need to pass through quotes, we should add an optional argument to addNavigationStep to indicate the string is already escaped (in which case we should only validate it).

If we're saying that addNavigationStep() receives a TypeScript-like expression, then .addNavigationStep("[X") should throw an exception. It is not a valid expression. Silently adding quotes to it will hide an escaping mistake in the caller, which is likely to cause even more confusion. In my example above, API Extractor is escaping incorrectly. We need to fix that, not patch it up in one particular code path.

TypeScript only allows late-bound symbols that can be referenced via an entity name, so only a or a.b.c are allowed. Element access is disallowed in an entity name, so a.[b]."[c]" wouldn't be supported.

If we pass this through to addNavigationStep() directly then "[e]" will get misinterpreted.

If that's true, then we should also always quote anything with [] and require the user to provide a value to an optional argument indicating the user did the escaping work. I will have something for that in an update shortly.

I pushed an update to PR microsoft/rushstack#1406 showing how the output looks with the new selectors.

I'm investigating how hard it is to get API Extractor to make the names like this:

export declare class X { "a": string; // "name": "a" "b[": string; // "name": "\"b[\"" [c]: string; // "name": "[c]" d: string; // "name": "d" "[e]": string; // "name": "\"[e]\"" }

It may need to be a separate PR, since there seem to be a number of places where names are built (and currently only 2 places consider symbols).

Heh quoting can be declaration-specific:

class X { public "f1"(x: string): void; public f1(x: boolean): void; public 'f1'(x: string | boolean): void { } }

This seems to work okay (using StringChecks.explainIfInvalidUnquotedIdentifier:

if (StringChecks.explainIfInvalidUnquotedIdentifier(followedSymbol.name)) { localName = JSON.stringify(followedSymbol.name); //<==== } else { localName = followedSymbol.name; } if (TypeScriptHelpers.isWellKnownSymbolName(followedSymbol.name)) { // TypeScript binds well-known ECMAScript symbols like "Symbol.iterator" as "__@iterator". // This converts a string like "__@iterator" into the property name "[Symbol.iterator]". localName = `[Symbol.${localName.slice(3)}]`; } else { const isUniqueSymbol: boolean = TypeScriptHelpers.isUniqueSymbolName(followedSymbol.name); for (const declaration of followedSymbol.declarations || []) { const declarationName: ts.DeclarationName | undefined = ts.getNameOfDeclaration(declaration); if (declarationName && ts.isIdentifier(declarationName)) { localName = declarationName.getText().trim(); break; } if (isUniqueSymbol && declarationName && ts.isComputedPropertyName(declarationName)) { const lateBoundName: string | undefined = TypeScriptHelpers.tryGetLateBoundName(declarationName); if (lateBoundName) { localName = lateBoundName; break; } } } }

If you don't see any problems with it, I'll open a separate PR that moves this name-calculator into a utility, and fixes up everywhere that names get made.

Seems like symbols and malformed identifiers can only occur as (possibly static) members of a class or interface. Thus there are less places to worry about than I expected.

Here's the PR: microsoft/rushstack#1410

octogonz

See notes

tsdoc/src/beta/DeclarationReference.ts

octogonz · 2019-07-22T08:21:19Z

This update looks good. Post a comment whenever it's ready and we'll get it merged+published.

Changes to BracketedComponent syntax

rbuckton · 2019-07-22T19:09:30Z

I'm done with changes for this PR.

octogonz · 2019-07-22T19:43:18Z

Before merging let me finish testing it against my API Extractor branch.

octogonz · 2019-07-23T01:03:18Z

As planned, API Extractor PR microsoft/rushstack#1406 now is generating JSON fields like this:

export declare class X {
    "a": string;    // "name": "a"
    "b[": string;   // "name": "\"b[\""
    [c.d]: string;  // "name": "[c.d]"
    e: string;      // "name": "e"
    "[f]": string;  // "name": "\"[f]\""
}

In the future the name will be generalized into a nesting JSON expression, but for right now the field is a text string (and is now consistently escaped).

@rbuckton But now I cannot figure out how to use your API to assemble these names:

const goal = DeclarationReference.parse('package!"a-b".[c.d].e');
console.log(goal.toString()) // package!"a-b".[c.d].e

const inputNames = ["package", '"a-b"', '[c-d]', 'e' ];

const step0 = DeclarationReference.package(inputNames[0]);
console.log(step0.toString()) // package!

const step1 = step0.addNavigationStep(Navigation.Exports, inputNames[1]);
console.log(step1.toString()) // package!"\"a-b\""

const step2 = step1.addNavigationStep(Navigation.Members, inputNames[2]);
console.log(step2.toString()) // package!"\"a-b\""#"[c-d]"

const step3 = step2.addNavigationStep(Navigation.Members, inputNames[3]);
console.log(step3.toString()) // package!"\"a-b\""#"[c-d]"#e

We want for step3.toString() to reproduce the same result as goal.toString() (without making any changes to inputNames).

Seems like the code below should fix it by unescaping step1, step2, and step3, but it now gets misinterpreted as an ECMAScript symbol reference:

const inputNames = ["package", '"a-b"', '[c-d]', 'e' ];

const step0 = DeclarationReference.package(inputNames[0]);
console.log(step0.toString()) // package!

const step1 = step0.addNavigationStep(Navigation.Exports, DeclarationReference.parse(inputNames[1]));
console.log(step1.toString()) // package!["a-b"]

const step2 = step1.addNavigationStep(Navigation.Members, DeclarationReference.parse(inputNames[2]));
console.log(step2.toString()) // package!["a-b"]#[[c-d]]

const step3 = step2.addNavigationStep(Navigation.Members, DeclarationReference.parse(inputNames[3]));
console.log(step3.toString()) // package!["a-b"]#[[c-d]]#[e]

Is this a bug? I would expect step3.toString() === goal.toString().

octogonz · 2019-07-23T01:58:44Z

@rbuckton In my PR I was able to get the correct output by doing this:

ApiMethod.ts (ae-new-canonical-references branch)

  /** @beta @override */
  public buildCanonicalReference(): DeclarationReference {
    return (this.parent ? this.parent.canonicalReference : DeclarationReference.empty())
      .addNavigationStep(this.isStatic ? Navigation.Exports : Navigation.Members, this._getCanonicalReferenceName())
      .withMeaning(Meaning.Member)
      .withOverloadIndex(this.overloadIndex);
  }

ApiNameMixin.ts (ae-new-canonical-references branch):

    /** @internal */
    public _getCanonicalReferenceName(): string | DeclarationReference {
      const name: string = this.name;
      if (name[0] === '"') {
        return JSON.parse(name);
      }
      if (name[0] === '[') {
        // Unwrap the [] and parse the reference
        return DeclarationReference.parse(name.substr(1, name.length - 2));
      }
      return name;
    }

The resulting UIDs look pretty good. So we just need to sort out this API issue and then we can merge both PRs.

rbuckton · 2019-07-23T04:38:42Z

Passing DeclarationReference.parse to addNavigationStep is for computed properties (it surrounds the reference with []). Try just pass the string to addNavigationStep? Also, I think by encoding "[b" as "\"[b\"", you will end up double escaping it now.

octogonz · 2019-07-23T06:05:36Z

Consider this excerpt from api-documenter-test.api.json from my PR:

{
  "kind": "PropertySignature",
  "canonicalReference": "api-documenter-test!IDocInterface3#[EcmaSmbols.example]:member",
  "docComment": "/**\n * ECMAScript symbol\n */\n",
  "excerptTokens": ...,
  "releaseTag": "Public",
  "name": "[EcmaSmbols.example]",
  "propertyTypeTokenRange": {
    "startIndex": 5,
    "endIndex": 6
  }
},

What kind of thing is the name field? Let's call it a name expression. What is its syntax?

Allowable values:

[EcmaSmbols.example]
x
"invalid-identifier"
[my-package!EcmaSmbols.example] (hypothetically)

Forbidden values:

a.b
@myscope/my-package!myMember

Thus although a name expression string looks like a DeclarationReference string, and can be parsed as one, it is technically a subcomponent of a declaration reference. In your API, addNavigationStep() conceptually accepts a name expression. It seems very strange for addNavigationStep() to accept DeclarationReference and then add [ ] around it. If I want to append something to p!X to produce p!X.[EcmaSmbols.example], it really feels like that thing I'm adding should be [EcmaSmbols.example] not EcmaSmbols.example. And thus its type should be a class like NameExpression not DeclarationReference.

API Extractor generates a name expression using custom code in AstSymbolTable.getLocalNameForSymbol(). For now, this code doesn't need any help: The symbol strings are made by TypeScriptHelpers.tryGetLateBoundName() and we won't change that for now. The string quoting is performed by JSON.stringify() based on StringChecks.isSafeUnquotedMemberIdentifier() which is stricter criteria than what TSDoc requires. So I think AstSymbolTable.getLocalNameForSymbol() is in good shape.

The awkward part is _getCanonicalReferenceName(), which parses this name expression and supplies it to addNavigationStep(). My hacky parsing feels wrong because it duplicates something that your Parser already handles.

How would you feel about these changes?

Define a type that corresponds to name expression. Component might be the right thing, but I can't figure out how to make that.
Expose an API that parses the name expression using your Parser
Change addNavigationStep() to accept a name expression. The way it handles string right now (by adding quotes if needed) is perfectly good. But it should also be able to receive [EcmaSmbols.example] and a redundantly quoted "x" as well, wrapped up as some class. addNavigationStep() should NOT accept DeclarationReference at all.

rbuckton · 2019-07-23T06:15:42Z

Define a type that corresponds to name expression. Component might be the right thing, but I can't figure out how to make that.

That's actually how it works. Generally you would pass either a ComponentString (for a regular property or quoted property), or a ComponentReference (for a DeclarationReference wrapped in []), however you can also pass in a string or a DeclarationReference and they are wrapped in a ComponentString or ComponentReference respectively, as a convenience feature.

octogonz · 2019-07-23T06:22:08Z

How to parse a Component?

rbuckton · 2019-07-23T06:32:44Z

A Component is either a ComponentString or a ComponentReference:

Component - https://github.com/microsoft/tsdoc/pull/173/files#diff-da116d95962e72d78bb0ebf1601d2785R307
ComponentString - https://github.com/microsoft/tsdoc/pull/173/files#diff-da116d95962e72d78bb0ebf1601d2785R339
ComponentReference - https://github.com/microsoft/tsdoc/pull/173/files#diff-da116d95962e72d78bb0ebf1601d2785R357

octogonz · 2019-07-23T11:11:52Z

function parseComponent(s: string): Component {
  if (s[0] === '[') return ComponentReference.parse(s);
  return new ComponentString(s);
}

Like this? Should this be a member of DeclarationReference?

octogonz · 2019-07-24T00:26:31Z

I added the parseComponent() API, since it addressed my problem.

octogonz · 2019-07-24T00:27:15Z

@rbuckton FYI I noticed a bug that DeclarationReference.parse("a b") does not report an error. We can address that as a separate issue, though.

Update DeclarationReference

23d02a2

rbuckton requested review from iclanton and octogonz as code owners July 22, 2019 03:07

rbuckton mentioned this pull request Jul 22, 2019

[api-extractor] Add an experimental new ApiItem.canonicalReference property microsoft/rushstack#1406

Merged

octogonz reviewed Jul 22, 2019

View reviewed changes

octogonz requested changes Jul 22, 2019

View reviewed changes

Update meanings, add 'userEscaped' argument

1802ed0

octogonz reviewed Jul 22, 2019

View reviewed changes

tsdoc/src/beta/DeclarationReference.ts Outdated Show resolved Hide resolved

rbuckton added 3 commits July 22, 2019 01:52

Move throw out of Parser

52143bc

Changes to BracketedComponent syntax

e793849

Merge pull request #1 from rbuckton/declarationReferenceUpdate2

7b8e262

Changes to BracketedComponent syntax

Add DeclarationReference.parseComponent() API

e6efcf1

octogonz approved these changes Jul 24, 2019

View reviewed changes

octogonz merged commit d8a564e into microsoft:master Jul 24, 2019

rbuckton deleted the declarationReferenceUpdate branch August 3, 2019 00:51

Adds ':call', ':new', and ':index' and fixes a parsing bug. #173

Adds ':call', ':new', and ':index' and fixes a parsing bug. #173

Uh oh!

Conversation

rbuckton commented Jul 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octogonz Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octogonz Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octogonz Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octogonz Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octogonz Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octogonz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

octogonz commented Jul 22, 2019

Uh oh!

rbuckton commented Jul 22, 2019

Uh oh!

octogonz commented Jul 22, 2019

Uh oh!

octogonz commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

octogonz commented Jul 23, 2019

Uh oh!

rbuckton commented Jul 23, 2019

Uh oh!

octogonz commented Jul 23, 2019

Uh oh!

rbuckton commented Jul 23, 2019

Uh oh!

octogonz commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Jul 23, 2019

Uh oh!

octogonz Jul 22, 2019 •

edited

Loading

octogonz Jul 22, 2019 •

edited

Loading

rbuckton Jul 22, 2019 •

edited

Loading

octogonz Jul 22, 2019 •

edited

Loading

octogonz Jul 22, 2019 •

edited

Loading

octogonz Jul 22, 2019 •

edited

Loading

octogonz commented Jul 23, 2019 •

edited

Loading

octogonz commented Jul 23, 2019 •

edited

Loading