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

Feature Request: Pattern Matching #696

Closed
hediet opened this issue Sep 2, 2019 · 8 comments
Closed

Feature Request: Pattern Matching #696

hediet opened this issue Sep 2, 2019 · 8 comments

Comments

@hediet
Copy link

hediet commented Sep 2, 2019

First of all: Thank you for this project! I really like it and I think it deserves more attention.
Something like this should be well known by every Typescript developer to automate medium to large refactorings. It is so sad that TypeScript's compiler API and its AST is such a mess.

Now to my actual feature request: It would be really great, if ts-morph has some kind of pattern matching support finding certain code patterns without much effort.

In my case, I want to refactor

function test(args: { id: string }) {
}
test({ id: "test" })

into

function test(id: string) {
}
test("test")

I know this transformation could be implemented as a generic refactoring, however, that is very complicated and I want a quick solution that mostly works and only has some edge cases left for manual edits.
To do that, it would be very handy if one could do something like this:

const m = match(
	// the node to match against
	node,
	pattern.call({
		expression: pattern.identifier('test'),
		args: [
			pattern
				.objectLiteral({
					// the id can be anything and is bound to "id"
					id: pattern.any().named('id')
				})
				// the object literal is bound to "arg"
				.named('arg'),
		],
	})
);
if (m) {
	// m.vars is typed
	m.vars.arg.replaceWith(m.vars.id);
}

What do you think?

It then would be very handy to have a Pattern.from(node) function that generates a pattern that only matches to an AST structurally equal to node. This can then be used to customize the pattern and add the necessary degree of freedom.

I know there is tsquery for quering the AST, but that is not very handy to use.

@hediet
Copy link
Author

hediet commented Sep 3, 2019

I've implemented a prototype by now that is already quite useable:

image

This is the binary expression being matched:

messageData[en][KEY_1] = 'message 1';

Sadly, matching AST nodes is not well typed :(

@lazarljubenovic
Copy link
Contributor

You might wanna check out tsquery.

@hediet
Copy link
Author

hediet commented Sep 4, 2019

As said, I already tried tsquery but it was not powerful enough for me: Its not clear how to easily match against complex trees and how to extract the data I want at given positions.

@dsherret
Copy link
Owner

Hey @hediet, sorry for my delay in replying.

I think pattern matching is very useful, but I'd rather it be separate from the library. See #585 and #351.

Basically, it's a lot of work to maintain and get something like that right.


Regarding your comment about the TypeScript compiler API, the compiler API has the specific purpose of compiling TypeScript code and supporting the user's experience in the editor. It wasn't really designed for programmatic code changes like what this library does (basically this library edits the source file text directly whereas when someone deals with the compiler api they would mostly be dealing with the AST).

@hediet
Copy link
Author

hediet commented Sep 21, 2019

@dsherret thanks for your reply ;)

Do you have an idea how to get the pattern matching fully typed?
Your (I assume hand written) AST facades are not very suitable for implementing typed patterns (e.g. using mapped/conditional types), as they all have getters and setters.

I noticed you have another typed data structure representing TypeScript ASTs (I think for generating ts code). Would that be suitable? Did you write these types by hand?

Btw. you can find my pattern matching prototype here.
I'll think of putting it into an external library.

However, I also need pattern matching on plain typescript ASTs for the refactorings I want to implement through language service plugins. Abstracting from ts-morph and typescript will be quite hard without code duplication, I yet have to come up with a solution.


Btw.: I did an intership at Microsoft a couple of years ago where I implemented a refactoring engine for a C# port of the typescript compiler. They ported the TypeScript AST types to various C# interfaces and a very cruel Union<T1, ..., TN> type. During my work there, I basically implemented something quite similar to ts-morph, but went a different route on how to treat modifications: I tried to reverse-build a nice ST out of the ugly AST so that refactorings could directly manipulate the ST without losing formatting. I kept a modification counter to keep symbols up to date.
Your approach of reparsing the file on each change has the advantage that invalid modifications are reported very early but has a great performance impact.
I implemented a code generator to get the typed ST. This code generator enabled some really cool features - for example, I checked during generation which parent types a node could be a child of to strengthen the node's parent type. I also had a very cool ST builder API.

I think that if the TypeScript team (even if I am eternaly greatful to them for making javascript usable) were also using a code generator for their AST types or were using a much more regular AST, they could both have a very performant AST but also enable many other usecases like ts-morph without facading everything.

Roslyn also managed to have a fast compiler but also a very nice compiler API.

@dsherret
Copy link
Owner

Do you have an idea how to get the pattern matching fully typed? Your (I assume hand written) AST facades are not very suitable for implementing typed patterns (e.g. using mapped/conditional types), as they all have getters and setters.

I feel like it should still be possible? Could you present a specific simplified example? I unfortunately don't have the time right now to really dive into this.

I noticed you have another typed data structure representing TypeScript ASTs (I think for generating ts code). Would that be suitable? Did you write these types by hand?

No, those wouldn't be suitable for traversing the AST or doing any matching. They don't have expressions yet (#683). Yeah, they're written by hand... they were originally just meant to be used for the #addClass kind of methods, but they've expanded so people can do stuff like sourceFile.addClass(classDec.getStructure()) or make changes to the structure before moving it over.

I did an intership at Microsoft a couple of years ago where I implemented a refactoring engine for a C# port of the typescript compiler.

Oohhh... I am mostly only a C# dev. That sounds kind of painful and prone to being out of sync with the actual TypeScript compiler...

Your approach of reparsing the file on each change has the advantage that invalid modifications are reported very early but has a great performance impact.

Yup exactly. The performance impact is pretty huge, but there are ways to speed things up. Over time I'd like to maintain the current API, but eliminate having to do this, but it would require a lot of changes in the TypeScript compiler API (basically, I would need a lot of internals to update... I thought about updating them myself anyway, but that would probably be a disaster in the long run). For example, see #610.

I implemented a code generator to get the typed ST. This code generator enabled some really cool features - for example, I checked during generation which parent types a node could be a child of to strengthen the node's parent type. I also had a very cool ST builder API.

Nice! Is this still being used?

I think that if the TypeScript team (even if I am eternaly greatful to them for making javascript usable) were also using a code generator for their AST types or were using a much more regular AST, they could both have a very performant AST but also enable many other usecases like ts-morph without facading everything.

Yup, that's true!

@hediet
Copy link
Author

hediet commented Sep 22, 2019

I feel like it should still be possible? Could you present a specific simplified example? I unfortunately don't have the time right now to really dive into this.

I'll do that in the coming days.

Oohhh... I am mostly only a C# dev. That sounds kind of painful and prone to being out of sync with the actual TypeScript compiler...

How come you invested so much effort in ts-morph when you are primary a C# dev?
Btw. they published the C# port here.

Nice! Is this still being used?

Sadly, I don't now, it has not been published. I tried to open source my work there, but it failed due to bureaucracy...

@dsherret
Copy link
Owner

I'll do that in the coming days.

Cool thanks! I'll try to sit down and take a closer look at this too.

Btw. they published the C# port here.

Wow!! I really wonder why that was done. Thanks for sharing that!

How come you invested so much effort in ts-morph when you are primary a C# dev?

At my old job in 2014 I worked a bit with asp.net (c#) so I did a bit of front end work because I had to. I got really excited about TypeScript after my boss showed me it and we adopted it right away. Since 2016 I've mostly been working on a c# desktop application (winforms...... luckily I didn't have to spend too much time dealing with the front end), but within the past few months I've transitioned over to a new project that's c# dealing with hardware. I don't work on the front end for that, but it's done with TypeScript.

Basically, I really love c# and TypeScript, but I don't like front end development. So I've ended up working primarily with c# at work, then I work on these TypeScript libraries in my spare time.

Sadly, I don't now, it has not been published. I tried to open source my work there, but it failed due to bureaucracy...

Too bad. 😞 I wonder if they would be more open to it nowadays.

@dsherret dsherret closed this as completed Oct 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants