Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Module declarations analysis support #19

Closed
guybedford opened this issue Jul 3, 2024 · 12 comments
Closed

Module declarations analysis support #19

guybedford opened this issue Jul 3, 2024 · 12 comments

Comments

@guybedford
Copy link
Collaborator

guybedford commented Jul 3, 2024

Module analysis will need to expand to support future module syntax features.

In particular, module declarations will likely introduce the concept of direct module linkage:

module x {
  import y;
}
module y {
  console.log('y');
}
import(x); // logs 'y'

The question for the analysis is then whether we include this import in the analysis data or not when checking x.imports().

We could have a special entry for the import like { type: 'declaration', declarationIdentifier: 'y' }.

Alternatively we could even include the direct declaration object in this analysis - { type: 'declaration', module: ModuleSource Y { ... } }.

We should ensure we have a good answer here that aligns with common future use cases.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jul 4, 2024

I would not expose those "direct" internal imports in the reflection API, since they are all bindings-based and not something that the host can control. It is an internal implementation detail of the module. And even if we expose them, I would absolutely not expose the name.

What we need to figure out is how to represent the external imports inside a module declaration:

module x {
  import "a";
}
import "b";

Does this module have "a" and "b" as imports? "b" and "a"? Does it have only "b"?

@nicolo-ribaudo
Copy link
Member

One option is to pre-resolve external imports in local imported module declaration, and provide more data for exported declarations. For example:

import { x } from "./a";
module y {
  import "./b";
}
export module z {
  import "./c";
}

import y;
import "./d";
import x;

could be:

mod.imports() == [
  { "specifier": "./a" },
  { "specifier": "./b" },
  { "specifier": "./d" },
  { "specifier": "./a", "module": "x" }
];
mod.namedExports() == ["z"];
mod.wildcardExports() == [];

const modZ = mod.getExportedModule("z");
modZ.imports() == [
  { "specifier": "./c" }
]

This exposes info about the exported/imported modules, since they affect linkage of files, while keeping internal-only modules that the host will never actually see or interact with as an internal unexposed details.

@guybedford
Copy link
Collaborator Author

guybedford commented Jul 4, 2024

I would say that until the module is explicitly imported, the dependency of a module declaration is not a dependency of a module itself.

That is, it is possible to have:

import 'z';
module x {
  import 'this-is-never-ever-imported';
}

where because there is no import(x) and the module x is not exposed externally, the import is never imported.

Therefore, if anything, I think a new analysis property may be useful to interrogate module expressions within a module source:

module.imports() == [{ specifier: 'z' }]
module.declarations() = [ModuleSource x {
  imports() // = [{ specifier: 'this-is-never-ever-imported' }]
}]

Where arbitrary nesting can also be inferred by recursively checking for declarations() on the declarations themselves.

@ljharb
Copy link
Member

ljharb commented Jul 4, 2024

Why would a non exported/imported module declaration - this, a completely unused one - need analysis at all?

@guybedford
Copy link
Collaborator Author

Yeah without knowledge of the name and linkage that information may be unnecessary, but one can imagine wanting to analyze bundles to know what modules they contain.

@ljharb
Copy link
Member

ljharb commented Jul 4, 2024

It shouldn’t be observable that I’ve added unused code, just like it’s not observable if i add an unused function declaration.

@guybedford
Copy link
Collaborator Author

guybedford commented Jul 4, 2024

Right, perhaps we only want exported declarations to be observable then.

It's just hard to trace if there's indirection:

module mod {
}
const x = mod;
export { x };

@ljharb
Copy link
Member

ljharb commented Jul 4, 2024

exported, or imported, presumably. also i'm not sure why that's hard to trace within a single file?

@guybedford
Copy link
Collaborator Author

Actually, the ability to import a module declaration only works across static bindings anyway, so the above won't work for module declarations:

// mod.js
module mod {
}
const x = mod;
export { x }

// main.js
import { x } from './mod.js';
import x; // THROWS "ReferenceError: Module declaration not statically defined" (or something like that)
import(x); // WORKS

That is, statically importable module declarations must be static bindings.

Perhaps it's okay then that only statically exposed (since exposed is now a well-defined property) module declarations are possible to gather analysis for?

@guybedford
Copy link
Collaborator Author

guybedford commented Jul 4, 2024

All this to say, perhaps the API is even on exports itself (in the static exported declaration case)?

module.exports() = [{ exportName: 'x', declaration: ModuleSource mod { } }]

@guybedford
Copy link
Collaborator Author

guybedford commented Jul 4, 2024

To complete the discussion here, considering the analysis of:

module x {
}
import { yMod as z } from 'y';

import x;
import z;

I agree with @nicolo-ribaudo that the name should be an internal detail, but the yMod name is useful to know in cross-bundle analysis per the discussion about exposed declarations above.

So in the above, maybe we can say that the module x {} is fully private from a public API perspective so that only the import z is something about which there is analysis available?

I'm then wondering if we might still extend imports analysis to provide a separate declarationImports() analysis, perhaps along the lines of:

mod.declarationImports() = [{ importName: 'yMod', specifier: 'y' }]

@guybedford
Copy link
Collaborator Author

Analysis has now been removed from the proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants