RFC: Extensibility model #9038

weswigham · 2016-06-08T23:40:21Z

We use our internal module resolution logic to resolve the location of the entrypoint of a plugin from the compilation, then pass that path off to the host to have it load the code. For our node host, this is just a require call. We then look at the exported values of this module and check for members annotated with the private member ["extension-kind"] which is intended to be one of the string literal values defined as valid ExtensionKinds. Later on, the loaded extensions can be retrieved and constructed when required, then have their methods called.

Please note: The extension API is not included anywhere in the compiler - however, an example of how to implement a package for using the extension API is given in the tests. It's my recommendation that the extension API be a separate package from TS proper, since the 'global' tsc needs to use the 'local' extension, and if that local extension has its own bundled typescript, then version mismatching can happen unexpectedly.

Additionally, to serve as a testbed for extension loading (and to implement a frequently asked for feature), single-pass semantic and syntactic lint extensions are also in this PR.

Thoughts:

We could implement the loadExtension method on other hosts similarly to how I've implemented it in the tests (if that is desirable). This is pretty much amounts to building a commonjs module loader into the compiler.

Future work:

'discoverTypings' hook
Language Service plugins (now in this PR)
Add quick fix support to lint plugins
Type Builder API
Type Relationship API
Type provider plugins (requires the above two)

weswigham · 2016-06-08T23:45:19Z

@mhegazy Would you like to look over this?

weswigham · 2016-06-17T21:37:54Z

@billti You should take a look at this, too.

weswigham · 2016-06-21T01:11:56Z

@alexeagle I am. I was asked to start working on this a bit more than two weeks ago. ;)

weswigham · 2016-06-21T01:34:27Z

I'm also planning on proposing an external API for building types, to pair with the language service extensions, type provider extensions (more of a prerequisite), and semantic lint extensions - this way you don't have to inject new files of types into a compilation (usually meaning re-running the compilation) just to perform type comparisons.

alexeagle · 2016-06-21T01:46:28Z

Other extension points that Angular needs:

codegen: create additional files before typechecking, write them to a configured gen directory, and include into the program before typechecking
some way to hook into the upcoming transforms pipeline to pre- and post-process the AST during emit.

I wrote more details: #3003 (comment)

jkillian · 2016-06-21T18:12:32Z

This looks very neat! I pulled these examples of a Lint extension out of your PR for ease of viewing:

import {SemanticLintWalker} from "typescript-plugin-api";

export default class IsValueFoo extends SemanticLintWalker {
    constructor(state) { super(state); }
    visit(node, stop, error) {
        const type = this.checker.getTypeAtLocation(node);
        if (type.flags & this.ts.TypeFlags.StringLiteral) {
            if (node.text === "foo") {
                error("String literal type 'foo' is forbidden.", node);
            }
        }
    }
}

import {SyntacticLintWalker} from "typescript-plugin-api";

export default class IsNamedFoo extends SyntacticLintWalker {
    constructor(state) { super(state); }
    visit(node, stop, error) {
        if (node.kind === this.ts.SyntaxKind.Identifier) {
            if (node.text.toLowerCase() === "foo") {
                error("Identifier 'foo' is forbidden.", node);
            }
        }
    }
}

jkillian · 2016-06-21T18:16:40Z

Skimming over this briefly, I didn't see an example of how an end user of an extension would be able to configure an extension, but it looks like from the code that it would be something like this in a tsconfig.json file:

"extensions": {
  "some-extension": { "option1": 1, "option2": 2},
  "no-options-extension": true,
  "any-data-format-extension": [1,2,3,4]
}

jkillian · 2016-06-21T18:19:41Z

src/compiler/program.ts

+            }
+
+            function error(err: string, node: Node) {
+                diagnostics.push(createExtensionDiagnosticForNode(node, activeLint.name, err));


There are times when an error might not appropriately be matched to a specific node. Is it worth considering a method of specifying an error that doesn't correspond to a single node?

Eh. I can just make node optional and emit a general program diagnostic if it's not present.

I think a reasonably easy way to provide the start and length of the added Diagnostic should give the appropriate amount of flexibility I think. Being able to provide a node (like the current implementation) seems like a special case of above, but on that's worth providing first-class support for as well.

function error(err: string); function error(err: string, node: Node); function error(err: string, start: number, length: number); function error(err: string, nodeOrStart?: Node | number, length?: number) { //... }

seems appropriate.

weswigham · 2016-06-22T00:20:29Z

@jkillian That would be correct. This should also be valid (if you don't need/want to pass any arguments to your extensions): extensions: ["ext-a", "ext-b"].

mhegazy · 2016-06-27T18:39:38Z

src/compiler/checker.ts

@@ -17258,6 +17258,10 @@ namespace ts {
                return getTypeForVariableLikeDeclaration(<VariableLikeDeclaration>node.parent, /*includeOptionality*/ true);
            }

+            if (node.kind === SyntaxKind.SourceFile) {


if the file is a module, we should return the module symbol.

sandersn · 2016-07-15T22:35:44Z

src/compiler/extensions.ts

+                            return; // Avoid errors on explicitly exported null/undefined (why would someone do that, though?)
+                        }
+                        const annotatedKind = potentialExtension["extension-kind"];
+                        if (typeof annotatedKind === "string") {


annotatedKind [](start = 35, length = 13)

when is this guard ever false? I thought "extension-kind" had a string literal union type.

We're reading it from untrusted JS. So it's always possible (at run time). This is where the canonical extension object is "made", so we need to guard against exceptional types here.

sandersn · 2016-07-21T16:00:29Z

src/compiler/program.ts

+                        Debug.fail("If only one argument is passed to error, it must be the error message string.");
+                        return;
+                    }
+                    // error(err: string): void;


does this comment belong here any more?

Yes - I was placing comments with the corresponding overload next to their implementation, just for clarity.

weswigham · 2016-07-21T19:12:41Z

@sandersn I think you'll be much more pleased with the error function's structure now (I know I am). The only downside is that there are more casts, since TS can't track throws across function calls. :(

sandersn · 2016-07-21T21:00:45Z

src/services/services.ts

+        // Overrides
+
+        // A plugin can implement one of the override methods to replace the results that would
+        // be returned by the TypeScript language service. If a plugin returns a defined results


results [](start = 89, length = 7)

typo: result

sandersn · 2016-07-21T22:06:09Z

src/services/services.ts

+        // program. The value passed in as previous is the result returned by the prior plugin. If a
+        // plugin returns undefined, the result passed in as previous is used and the undefined
+        // result is ignored. All plugins are consulted before the result is returned to the host.
+        // If a plugin overrides behavior of the method, no filter methods are consulted.


If a plugin overrides behavior of the method, no filter methods are consulted. [](start = 10, length = 79)

I'm confused by the relationship of override and filter methods. Are overrides and filters called in the same sequence? It sounds like they are, with the first override aborting the cascade of the previous filters (but not throwing away their filtered values).

What happens if a plugin has both an override and a filter?

Could the whole thing be simplified to just have filter that have some way to signal that they should be the last plugin in the cascade -- that is, that they are an override? (This would dodge (1) by only allowing one of each on a class.

If an override returns a non-undefined value, no further overrides are queried, no filters are queried. and the value is returned. If no override returns a non-undefined value, then the base service is queried for a value. This value is then piped through the filter chain. If a filter returns undefined, the previous value is used as input to the next filter instead. The result of the last non-undefined returning filter is the final result, returned by the service.

A plugin can have both an override and a filter because it may, at times, wish to override everything (and at other times returning undefined and not providing a value) and at others filter (assuming it returned undefined for the same operation in its override method).

So overrides are before (or replace) and filters are after, but overrides stop filters from running?

I'd suggest two things:

Document overrides and filters together, plus add an example or two to show how they interact. (Maybe even change the names.)

Consider making it so that overrides don't stop filters. Wouldn't it be useful for other plugins to be able to filter the output of a previous plugin?

Okay.

@chuckjaz had the original API idea. Unless he has some strong objection to this, I'm okay with always having filters run.

weswigham · 2016-08-04T22:15:33Z

After some discussion within the team, I'm going to split this PR back into three separate PRs: One for loading extensions, one for lint extensions, and one for LS extensions. We're generally find with loading extensions and how that works, however we have some contention within the team as to what extensions have value and what risks we're willing to take with them. I'm going to close this PR and open up three new ones shortly.

weswigham · 2016-08-05T02:10:07Z

This PR has now been split into #10159, #10160, and #10162.

TheLarkInn · 2016-08-05T22:20:17Z

Cc @TheLarkInn to track this incredible awesomeness. We could test this for webpack also.

msftclas added the cla-already-signed label Jun 8, 2016

weswigham changed the title ~~WIP: Extensibility model~~ RFC: Extensibility model Jun 9, 2016

weswigham force-pushed the extensibility-model branch 5 times, most recently from d077396 to f1b8fe0 Compare June 20, 2016 20:50

Functional extension loading & lint passes

8b3aeb8

weswigham force-pushed the extensibility-model branch from f1b8fe0 to 8b3aeb8 Compare June 20, 2016 23:30

weswigham mentioned this pull request Jun 20, 2016

RFC: Language service extensions and tests #9283

Merged

weswigham added 2 commits June 20, 2016 17:36

Dont look for extensions in a namespace

8583037

Language service extensions and tests

a7ae427

Fix comments - make behavior align with comments

a03e465

alexeagle mentioned this pull request Jun 21, 2016

Add optional type information to rules palantir/tslint#1323

Closed

jkillian reviewed Jun 21, 2016
View reviewed changes

weswigham added 2 commits June 22, 2016 10:54

accept -> stop, add more overloads to error

4eee39d

Merge branch 'extensibility-model' into language-service-extensions

bff6b05

mhegazy reviewed Jun 27, 2016
View reviewed changes

sandersn reviewed Jul 15, 2016
View reviewed changes

weswigham added 3 commits July 20, 2016 11:25

Merge branch 'master' into extensibility-model

4676ca2

Use new performance framework, remove extra compiler option, traces

b8ca16a

Merge branch 'master' into extensibility-model

563d71f

weswigham mentioned this pull request Jul 21, 2016

Code generation extensibility point #9849

Closed

sandersn reviewed Jul 21, 2016
View reviewed changes

Cleanup performLintPass from feedback from @sandersn

ae66e99

sandersn reviewed Jul 21, 2016
View reviewed changes

Fix typo

842d3c3

sandersn reviewed Jul 21, 2016
View reviewed changes

Merge branch 'master' into extensibility-model

805bc2f

weswigham force-pushed the extensibility-model branch from 62c8c17 to 805bc2f Compare August 1, 2016 20:35

weswigham added 2 commits August 1, 2016 14:53

Fix baselines with new AST, semantic lint test uses type instead of node

40b35c4

Make filters always run

e5c342a

weswigham closed this Aug 4, 2016

This was referenced Aug 5, 2016

Extension loading and management tools #10159

Closed

Language Service Extensions #10160

Closed

Lint extensions #10162

Closed

waderyan added the VS Code Tracked There is a VS Code equivalent to this issue label Sep 14, 2016

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Extensibility model #9038

RFC: Extensibility model #9038

weswigham commented Jun 8, 2016 •

edited

Loading

weswigham commented Jun 8, 2016 •

edited

Loading

weswigham commented Jun 17, 2016

weswigham commented Jun 21, 2016

weswigham commented Jun 21, 2016

alexeagle commented Jun 21, 2016

jkillian commented Jun 21, 2016 •

edited by weswigham

Loading

jkillian commented Jun 21, 2016

jkillian Jun 21, 2016

weswigham Jun 21, 2016

jkillian Jun 22, 2016

weswigham Jun 22, 2016

weswigham commented Jun 22, 2016

mhegazy Jun 27, 2016

sandersn Jul 15, 2016

weswigham Jul 15, 2016 •

edited

Loading

sandersn Jul 21, 2016 •

edited

Loading

weswigham Jul 21, 2016

weswigham commented Jul 21, 2016

sandersn Jul 21, 2016

sandersn Jul 21, 2016

weswigham Jul 21, 2016

sandersn Jul 21, 2016

weswigham Jul 21, 2016 •

edited

Loading

weswigham commented Aug 4, 2016

weswigham commented Aug 5, 2016

TheLarkInn commented Aug 5, 2016

RFC: Extensibility model #9038

RFC: Extensibility model #9038

Conversation

weswigham commented Jun 8, 2016 • edited Loading

weswigham commented Jun 8, 2016 • edited Loading

weswigham commented Jun 17, 2016

weswigham commented Jun 21, 2016

weswigham commented Jun 21, 2016

alexeagle commented Jun 21, 2016

jkillian commented Jun 21, 2016 • edited by weswigham Loading

jkillian commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham commented Jun 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham Jul 15, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Jul 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham Jul 21, 2016 • edited Loading

Choose a reason for hiding this comment

weswigham commented Aug 4, 2016

weswigham commented Aug 5, 2016

TheLarkInn commented Aug 5, 2016

weswigham commented Jun 8, 2016 •

edited

Loading

weswigham commented Jun 8, 2016 •

edited

Loading

jkillian commented Jun 21, 2016 •

edited by weswigham

Loading

weswigham Jul 15, 2016 •

edited

Loading

sandersn Jul 21, 2016 •

edited

Loading

weswigham Jul 21, 2016 •

edited

Loading