fix(check): compiler options from workspace members #27009
Conversation
|
Yeah this is not for landing yet |
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
nayeemrmn
changed the title
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
Cargo.toml
Outdated
| # TODO(nayeemrmn): Use proper version when https://github.com/denoland/deno_config/pull/143 lands! | ||
| deno_config = { git = "https://github.com/denoland/deno_config.git", rev = "0d588fb1831bf4d33d3279a1a75db6138d81a75b", features = ["workspace", "sync"] } |
There was a problem hiding this comment.
To address before landing
There was a problem hiding this comment.
Can you outline how this solution works on a high level?
Also, is there a way we could instead contain the solution to only being done when type checking a set of specifiers or a module graph? (ex. maybe it segments parts of the graph and then when type checking one area of the graph it uses certain rules and when type checking another it uses other rules) I'm not sure such foundational changes like creating multiple factories per execution are necessary to implement this.
| false, | ||
| scope_options.map(Arc::new), | ||
| )?); | ||
| let mut factory = CliFactory::from_cli_options(cli_options.clone()); |
There was a problem hiding this comment.
Ideally we create a single factory and then from that single factory we create services. Right now this seems to be doing a lot of setup code over and over for each workspace directory? Is there a way we could change this not to do that? Why is this necessary?
There was a problem hiding this comment.
Also responding to #27009 (review).
It's an implementation detail of this struct to avoid doing a larger refactor for now. The services in CliFactory are based on the config from the workspace directory in cli_options.start_dir, and deeply attached to cli_options.start_dir.workspace().
To expound on that, I want this to be initial infrastructure for cross-config support without workspaces (i.e. across multiple workspaces). That means it needs to support completely different resolution, vendor/cache locations and other graph-building settings across different workspace dirs. The LSP already supports this for reference.
Currently the resolve_foo_options_for_members() methods don't support cross-workspace, but the point is to not further entrench that limitation.
There was a problem hiding this comment.
I don't think we should support more than one workspace in most of the CLI code. I think that would lead to too much complexity and is a fair limitation to have in the CLI. I think we should remove that for now and focus on just having this work within one workspace.
There was a problem hiding this comment.
In the future, if we add support for multiple workspaces then we should do a larger refactor that specifically does that, but I think that's too much to add for this PR.
There was a problem hiding this comment.
I think we should remove that for now and focus on just having this work within one workspace.
I can plumb the compiler options through the check invocation rather than keeping it in the state, which would allow sharing the root factory for now I think. I'll see if everything still works with that.
I don't think we should support more than one workspace in most of the CLI code. I think that would lead to too much complexity and is a fair limitation to have in the CLI.
Strongly disagree, it's not fair that if users want their subproject configurations to be differentially respected, they need to use workspaces. To the contrary they should use workspaces when they have shared configuration to factor out. There's no more complexity in splitting args, invoking many times and optimising for shared state than we already have to do with single-workspace-many-members. It's just complex to refactor what we have now.
There was a problem hiding this comment.
If we do do that, then we could stick to keeping it all behind Workspace without changing anything in the CLI. We already support multiple workspaces there with the patch feature.
There was a problem hiding this comment.
I can plumb the compiler options through the check invocation rather than keeping it in the state, which would allow sharing the root factory for now I think. I'll see if everything still works with that.
Maybe in TypeChecker we could see the current ModuleGraph that gets passed in and from that segment the graph based on all the different kinds of compiler options found from the CliOptions and run for each set of compiler options. Then we wouldn't need to plumb the compiler options through I think.
There was a problem hiding this comment.
Okay I was reminded why this is an issue even in single-workspace, there is no shared ModuleGraph because in the very least JsxImportSourceConfig will vary with compiler options and is a resolver parameter.
We could only respect the one specified in the compiler options of the 'default' cli_options.start_dir (btw I hate this, there shouldn't be such a thing), but IIRC this was stressed as a "do it right or not at all" point during a meeting.
We could give the resolver BTreeMap<Url, JsxImportSourceConfig>, but we'd really be digging ourselves into the single-workspace assumption for this to affect deno_graph's API
It would be simplest and most correct to just handle this like editors do. If two files from different configuration scopes are specified at the CLI (shared workspace or not), assume little about what settings they share and construct different graph-building/cache services for each.
I'm looking over the code base and CliFactory is constructed relatively late in the process for each command. It's not even shared between watched runs for instance. Other than maybe its name there's nothing strange about having one-per-workspace-dir.
There was a problem hiding this comment.
We need any solution that doesn't create multiple factories and a solution that creates one graph. Introducing multiple factories into the CLI is too much complexity, will use too much memory, be slower (ex. loading modules multiple times), and will lead to bugs.
but we'd really be digging ourselves into the single-workspace assumption for this to affect deno_graph's API
A Workspace can be changed to support any number of deno workspaces. It already has a bit of support for this with the patch feature.
Okay I was reminded why this is an issue even in single-workspace, there is no shared ModuleGraph because in the very least JsxImportSourceConfig will vary with compiler options and is a resolver parameter.
Let's update deno_graph to ask based on the referrer. We need this anyway for deno run.
Let's get this fix landed with minimal foundational design changes and without creating multiple factories. Let's consider them out of scope for this PR.
Divide the passed files by the workspace directories they fall under, check each of these sets individually with their respective settings. Each of these only report diagnostics for the modules that belong to their respective workspace dir scopes, so if an erroneous module is imported cross-scope, its diagnostics won't be reported twice. This is what |
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
| pub struct WorkspaceBenchOptions { | ||
| pub filter: Option<String>, | ||
| pub json: bool, | ||
| pub no_run: bool, | ||
| } | ||
|
|
||
| impl WorkspaceBenchOptions { | ||
| pub fn resolve(bench_flags: &BenchFlags) -> Self { | ||
| Self { | ||
| filter: bench_flags.filter.clone(), | ||
| json: bench_flags.json, | ||
| no_run: bench_flags.no_run, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This was supposed to abstract options inferred from flags and deno.json configs, but currently just clones over the flags. I've removed it for now. The usage would need to change if it took into account deno.jsons because it may have to be per-workspace-dir, so the current usage would be confusing to keep.
| #[derive(Clone, Debug)] | ||
| pub struct WorkspaceTestOptions { | ||
| pub doc: bool, | ||
| pub no_run: bool, | ||
| pub fail_fast: Option<NonZeroUsize>, | ||
| pub permit_no_files: bool, | ||
| pub filter: Option<String>, | ||
| pub shuffle: Option<u64>, | ||
| pub concurrent_jobs: NonZeroUsize, | ||
| pub trace_leaks: bool, | ||
| pub reporter: TestReporterConfig, | ||
| pub junit_path: Option<String>, | ||
| pub hide_stacktraces: bool, | ||
| } | ||
|
|
||
| impl WorkspaceTestOptions { | ||
| pub fn resolve(test_flags: &TestFlags) -> Self { | ||
| Self { | ||
| permit_no_files: test_flags.permit_no_files, | ||
| concurrent_jobs: test_flags | ||
| .concurrent_jobs | ||
| .unwrap_or_else(|| NonZeroUsize::new(1).unwrap()), | ||
| doc: test_flags.doc, | ||
| fail_fast: test_flags.fail_fast, | ||
| filter: test_flags.filter.clone(), | ||
| no_run: test_flags.no_run, | ||
| shuffle: test_flags.shuffle, | ||
| trace_leaks: test_flags.trace_leaks, | ||
| reporter: test_flags.reporter, | ||
| junit_path: test_flags.junit_path.clone(), | ||
| hide_stacktraces: test_flags.hide_stacktraces, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This was supposed to abstract options inferred from flags and deno.json configs, but currently just clones over the flags. I've removed it for now. The usage would need to change if it took into account deno.jsons because it may have to be per-workspace-dir, so the current usage would be confusing to keep.
…ber-compiler-options
…ber-compiler-options
| false, | ||
| scope_options.map(Arc::new), | ||
| )?); | ||
| let mut factory = CliFactory::from_cli_options(cli_options.clone()); |
There was a problem hiding this comment.
We need any solution that doesn't create multiple factories and a solution that creates one graph. Introducing multiple factories into the CLI is too much complexity, will use too much memory, be slower (ex. loading modules multiple times), and will lead to bugs.
but we'd really be digging ourselves into the single-workspace assumption for this to affect deno_graph's API
A Workspace can be changed to support any number of deno workspaces. It already has a bit of support for this with the patch feature.
Okay I was reminded why this is an issue even in single-workspace, there is no shared ModuleGraph because in the very least JsxImportSourceConfig will vary with compiler options and is a resolver parameter.
Let's update deno_graph to ask based on the referrer. We need this anyway for deno run.
Let's get this fix landed with minimal foundational design changes and without creating multiple factories. Let's consider them out of scope for this PR.
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
Closes #24504.
Blocked by denoland/deno_config#143.