[WIP] Refactor the compiler #6894

kitsonk · 2020-07-27T11:58:43Z

This is a major refactor of the TypeScript compilation infrastructure. Raising for early visibility, but it is still very much a work in progress. It totally rewrites how the compilations are handled to abstract out the logic, which should make it easier to move parts and pieces in the future, at the same time leveraging more Rust infrastructure and signficantly "dumbing" down the interface to the TypeScript compiler.

Currently it is a seperate crate as that has been easier for me to work on it without having to deal with merge conflicts and other changes.

Currently I have the following working:

Ability to create snapshots with custom libs (like deno.ns).
Ability to compile a set of sources and specify any custom libs to type check against.
Ability to transpile an arbitrary set of sources.

Things that I need to do:

Introduce bundling, which the bundle creation will be done in Rust, using the TypeScript compiler just for type checking and transpilation.
Finish off the display of the diagnostics.
Look at moving the dependency graph logic in.
Integrate into the CLI.

When finished this will:

Fixes #5606
Fixes #5607
Fixes #6686

ry · 2020-07-27T16:21:42Z

Can you git mv the various typescript files so the diff was easier to look at?

CLAassistant · 2020-07-27T23:49:06Z

All committers have signed the CLA.

kitsonk · 2020-07-28T02:08:27Z

Can you git mv the various typescript files so the diff was easier to look at?

If I did that right now, it would break CLI, so I have simply hacked around it for now.

cli/tsc/00_typescript.js

kitsonk · 2020-08-15T05:02:30Z

It is getting closer and closer. Still to do:

Bundle needs to be refactored use Graph.
bring in the swc type stripping/transpiling
The diagnostics refactor needs to be completed.
import-map could use some cleanup and refactoring, maybe after this PR?
integrate with cli...
question: are redirects handled properly?
provide runtime APIs, likely will just go with the refactor proposed in Refactor runtime compiler APIs #4752 instead of trying to re-implement existing ones

Also, this will provide even more fixes:

Fixes #7005
Fixes #6176
Fixes #6995

bartlomieju

@kitsonk here are some of my initial questions, I need to go over this PR a few more times as it's very compilated. Also what's your plan regarding integration with CLI?

bartlomieju · 2020-08-17T10:18:21Z

compiler/Cargo.toml

+serde = { version = "1.0.115", features = ["derive"] }
+sourcemap = "6.0.1"
+swc_common = { version = "=0.9.1", features = ["sourcemap"] }
+swc_ecma_transforms = { version = "=0.19.7", features = ["react"] }


That's already part of swc_ecmascript given "transforms" feature

It wasn't... It was panic'ing. I will try again.

Yeah, validated it again, if I remove that, I get:

error[E0432]: unresolved import `swc_ecmascript::transforms::react` --> compiler/ast.rs:40:5 | 40 | use swc_ecmascript::transforms::react; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `react` in `transforms`

So it appears to not be included in swc_ecmascript and you have to include the other crate to have it be available.

CC @kdy1 is there a better way to this?

Sorry, I forgot adding cargo features to swc_ecmascript. I'm too good at forgotting thngs :(
I'll publish a patch soon.

compiler/ast.rs

bartlomieju · 2020-08-17T10:23:16Z

compiler/bundler.rs

+// Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
+
+use crate::source_map_bundler::SourceMapBundler;
+use crate::Result;
+
+use deno_core::ErrBox;
+use std::error::Error;
+use std::fmt;
+use std::path::PathBuf;
+


Sooo... swc_bundler is now available and I wanted to try and incorporate it into our workflow, but I guess we'll start with your version first and then try to switch to it. WDYT?

compiler/compiler.js

compiler/config.rs

compiler/module_graph.rs

bartlomieju · 2020-08-17T11:28:00Z

compiler/ast.rs

+use crate::msg::MediaType;
+use crate::Result;
+
+use deno_core::ErrBox;


Note to self, most of the contents of this file are used in cli/ (mainly for compiler purposes but also for deno doc) as well as in deno_lint. It should be made into a utility crate that is shared between the other crates.

bartlomieju · 2020-08-17T11:31:45Z

compiler/module_graph.rs

+use crate::ast::parse;
+use crate::ast::Location;
+use crate::ast::ParsedModule;
+use crate::compiler::CompileOptions;
+use crate::compiler::CompilerEmit;
+use crate::compiler::CompilerIsolate;
+use crate::compiler::TranspileOptions;
+use crate::import_map::ImportMap;
+use crate::msg::EmittedFile;


Note to self: ModuleGraph/Graph should be used for deno info, investigate if this struct could be made into two separate structs - one for loading and parsing of dependencies (think collecting module tree), second for integration with TS compiler cache (think utilizing the graph that collects module tree to process the tree further). See #6786 for reference

While it sort of overloads it a bit, I sort of like them together... In that Graph contains all the information that ops need to access files, you just wire it into that... I like having all the "what you can do with a graph" on the surface API of the graph...

For GraphBuilder there is only:

insert - Add a specifier as a "root".

get_graph - Move the graph out.

For Graph it currently is:

Updating a graph:

compile (update the graph with type checked code)

transpile (update the graph with type stripped code)

Things I need to refactor out/make non-public:

update (take emitted files and update the graph... I need to refactor this out, as compile and transpile should automatically do this, making the compiler less dependent)

flush (flush any "dirty" modules to the cache, again this should be refactored out as it need to be handled by compiler and transpile)

get_root_names (this was used by compiler.compile but needs to be passed)

get_sources (this was used by transpile but need to be passed)

Then needed for compiler ops:

pub modules (the Hashmap of modules)

resolve (takes a string specifier and a base module specifier and returns the module specifier in the graph, abstracting out all the logic for @deno-types and X-TypeScript-Types and triple-slash reference)

get_build_info/set_build_info

But talking that through here sounds like I should refactor that last group as a trait (and abstract out the public access to the hashmap of module).

Adding in info() to the Graph makes a lot of sense, and/or have the Display of a Graph be the output we need for info.

Ok, I have done a refactor... I still want to do some fine tuning of it, but basically the compiler and the graph are better decoupled, and the compiler ops only depend upon to methods in a trait. It means in practice, the only thing that needs to be interfaced with is the GraphBuilder and the Graph.

bartlomieju · 2020-08-17T11:37:10Z

compiler/import_map.rs

+
+use crate::Result;
+
+use deno_core::ModuleSpecifier;
+use indexmap::IndexMap;


ImportMap implementation hasn't really changed since it was introduced last year in May, maybe it would make more sense to make it a separate crate as well? It would still depend on deno_core for the sake of ModuleSpecifier, but could limit complexity by having a clear boundary between the crates. @kitsonk @ry what do you think?

I think it will "move" here, it shouldn't be needed by the CLI anywhere else. Though of course compiler is sort of becoming a bad name for this crate... but basically it is the all the higher order logic of dependency/handling/transforming/type checking of ES Modules under Deno, abstracted away from the fetching/caching/persistence/runtime execution considerations.

compiler/ast.rs

bartlomieju · 2020-08-17T11:57:20Z

question: are redirects handled properly?

At the moment I can't see any handling of redirects, I think we can punt on this issue for a bit longer. We need to take holistic approach and coordinate redirect handling in deno_core as well. See:

deno/core/modules.rs

Lines 40 to 44 in 684eddc

    
           // TODO(bartlomieju): I have a strong opinion we should store all redirects 
        
           // that happened; not only first and final target. It would simplify a lot 
        
           // of things throughout the codebase otherwise we may end up requesting 
        
           // intermediate redirects from file loader. 
        
           #[derive(Debug, Eq, PartialEq)]

kitsonk · 2020-08-27T22:31:25Z

In chatting with @ry and @bartlomieju we agreed that we need to break this up into seperate things and for right now, moving this logic out to a seperate crate doesn't make sense...

I will use this branch though as a reference implementation for myself (and for @bartlomieju) to land some/all of these changes over a period of time.

kitsonk mentioned this pull request Jul 27, 2020

Use SWC to strip types for "--no-check" flag #6895

Merged

kitsonk force-pushed the deno_compiler branch from 5d1bccf to 6294ebb Compare July 28, 2020 02:07

kitsonk force-pushed the deno_compiler branch 3 times, most recently from ead32a6 to 97252ec Compare July 29, 2020 07:32

ry reviewed Jul 30, 2020

View reviewed changes

cli/tsc/00_typescript.js Outdated Show resolved Hide resolved

kitsonk force-pushed the deno_compiler branch 2 times, most recently from 905f218 to c03b273 Compare August 12, 2020 21:31

lucacasonato mentioned this pull request Aug 13, 2020

Rust library with batteries included #2633

Closed

kitsonk force-pushed the deno_compiler branch 2 times, most recently from ea84031 to 3fd14f0 Compare August 14, 2020 20:26

kitsonk force-pushed the deno_compiler branch from 34ca1d3 to e3a0866 Compare August 17, 2020 06:31

bartlomieju reviewed Aug 17, 2020

View reviewed changes

bartlomieju mentioned this pull request Aug 17, 2020

Make Dino.bundle() work like dino bundle? #6734

Closed

bartlomieju reviewed Aug 17, 2020

View reviewed changes

compiler/ast.rs Outdated Show resolved Hide resolved

kitsonk force-pushed the deno_compiler branch 2 times, most recently from d75cb1d to 3f04a42 Compare August 21, 2020 07:45

bartlomieju mentioned this pull request Aug 22, 2020

no-check doesn't support some features #7080

Closed

kitsonk force-pushed the deno_compiler branch from 3f04a42 to f4841a0 Compare August 24, 2020 20:39

nayeemrmn mentioned this pull request Aug 26, 2020

Type Checking In Non TS Files #7196

Closed

kitsonk force-pushed the deno_compiler branch from f4841a0 to 8d59be4 Compare August 27, 2020 05:30

WIP

bd1c62e

kitsonk force-pushed the deno_compiler branch from 8d59be4 to bd1c62e Compare August 27, 2020 10:55

kitsonk closed this Aug 27, 2020

kitsonk mentioned this pull request Aug 27, 2020

Refactoring how we compile code #7225

Closed

13 tasks

bartlomieju mentioned this pull request Sep 3, 2020

fix: regex for TS references and deno-types #7333

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Refactor the compiler #6894

[WIP] Refactor the compiler #6894

kitsonk commented Jul 27, 2020

ry commented Jul 27, 2020

CLAassistant commented Jul 27, 2020 •

edited

Loading

kitsonk commented Jul 28, 2020

kitsonk commented Aug 15, 2020 •

edited

Loading

bartlomieju left a comment

bartlomieju Aug 17, 2020

kitsonk Aug 17, 2020

kitsonk Aug 18, 2020

bartlomieju Aug 18, 2020

kdy1 Aug 18, 2020

bartlomieju Aug 17, 2020

bartlomieju Aug 17, 2020

bartlomieju Aug 17, 2020

kitsonk Aug 19, 2020

kitsonk Aug 21, 2020

bartlomieju Aug 17, 2020

kitsonk Aug 19, 2020

bartlomieju commented Aug 17, 2020

kitsonk commented Aug 27, 2020

[WIP] Refactor the compiler #6894

[WIP] Refactor the compiler #6894

Conversation

kitsonk commented Jul 27, 2020

ry commented Jul 27, 2020

CLAassistant commented Jul 27, 2020 • edited Loading

kitsonk commented Jul 28, 2020

kitsonk commented Aug 15, 2020 • edited Loading

bartlomieju left a comment

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

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

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

Choose a reason for hiding this comment

bartlomieju commented Aug 17, 2020

kitsonk commented Aug 27, 2020

CLAassistant commented Jul 27, 2020 •

edited

Loading

kitsonk commented Aug 15, 2020 •

edited

Loading