feat(transformers): Add beforeDeclarations transformers

[Danielku15]

Fixes #58880

Relates to #17146 and #41486 (not fully fixes them)

The custom transformers were lacking a beforeDeclarations allowing devs to do a AST transformation before the built-in transformers. If you wanted to use information from erased AST nodes (visibility, initializers, etc.) you were lost. With this change devs can improve their JSDoc comments or use other transformations specific for declarations.

Sorry for the PR without Backlog milestone issue. I still hope this makes it through.

[typescript-bot]

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

[typescript-bot]

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

[sandersn]

To help with PR housekeeping, I'd prefer not to have PRs for bugs that aren't accepted yet. Discussion usually brings up new implementation requirements, and the codebase can change underneath the PR.

[typescript-bot]

The TypeScript team hasn't accepted the linked issue #58880. If you can get it accepted, this PR will have a better chance of being reviewed.

[Danielku15]

@sandersn Typically I would fully agree that discussion should happen before PRs are opened and features proposed. I still hope that this would be a low-hanging-fruit topic we can easily pick. IMHO it was an oversight to properly add "beforeDeclarations" when "afterDeclarations" transformers were added (considering normal before/after exists too). I really hope that not the whole functionality as such (pre-existing) is being challenged as part of an extension where 3/4 combinations are enhanced to ship 4/4.

As discussion around such topics often happens across years, and is spread across thousands of issues. It is hard to expect people will become active on a new issue. With this I hope the maintainers dive at least a bit into the issue and PR to reflect on it. Its really not such a big deal with direct added benefit. Also considering there is already a tree of partially related issues with interest.

[rbuckton]

Unfortunately, I would not classify this as "low-hanging fruit". I'm very wary about adding a beforeDeclaration transformer given that the current declaration transformer expects to receive the original parse tree, not a transformed tree, and may be making many assumptions about the provenance of nodes in the tree. Adding a "before" parser to the transforms required a fair amount of consideration and rethinking of the ts transformer to ensure it worked properly with a transformed tree. I'd like to see far more test coverage of various potential tree mutations and their effects on declaration output before I'd feel confident about making this change.

[rbuckton]

may be making many assumptions about the provenance of nodes in the tree.

For example, src/compiler/transformers/declarations.ts contains at least 38 references to .parent, but a transformed source tree does not have reliable .parent pointers, and that's not taking into account imported functions that might also access .parent.

[Danielku15]

I understand your concern, but how is beforeDeclarations much different to normal before transformers? There are also plenty of built-in before transformers which run after the custom transformers:

TypeScript/src/compiler/transformer.ts

Lines 133 to 139 in dc535a7

	addRange(transformers, customTransformers && map(customTransformers.before, wrapScriptTransformerFactory));
	transformers.push(transformTypeScript);
	if (compilerOptions.experimentalDecorators) {
	transformers.push(transformLegacyDecorators);
	}

With your argumentation before transformers should be removed because devs might create transformers producing a nodes incompatible with the built-in ones. Is your concern that you are held responsible for custom transformers breaking the built-in ones causing too much support?

[rbuckton]

I understand your concern, but how is beforeDeclarations much different to normal before transformers?

With your argumentation before transformers should be removed because devs might create transformers producing a nodes incompatible with the built-in ones.

You misunderstand my concern. Prior to adding custom before transforms, the ts transform also had numerous places where it expected to only receive an original parse tree. When we added custom before transforms, the ts transform had to be updated to be more resilient to custom transforms by avoiding .parent pointers, making calls to getOriginalNode as needed, etc.

As far as the other JavaScript-emitting transformers, they were all initially written with the expectation that they were receiving a transformed tree, so they made no assumptions about provenance and avoided these pitfalls.

If we want to add a beforeDeclarations transform, the declarations.ts transform must also be made resilient.

[Danielku15]

Thanks a lot for clarfiying, makes totally sense. Once I find some time for this topic again, I might dive a bit deeper into the transformer codebase to see how they are currently organized compared to the normal before runners.

Are there already some good tests for before transformers which try to break the built-in transformers I could look at?

[rbuckton]

Are there already some good tests for before transformers which try to break the built-in transformers I could look at?

We have a number of tests in src\testRunner\unittests\transform.ts and src\testRunner\unittests\customTransforms.ts that exercise custom before transforms.