Skip to content

Conversation

@chabad360
Copy link

@chabad360 chabad360 commented Jun 26, 2024

This PR allows for a bunch of errors to be detected by the built-in Go inspections, and by proxy, it adds support for highlighting as well as some other stuff. It's not perfect by any means (in fact, there are a number of glaring issues), but this is what happened when I went down the rabbit hole that started with #30.

Effectively fixes #26

@joerdav
Copy link
Collaborator

joerdav commented Jun 26, 2024

Thanks! I'll take a look. I'm also not very practiced in Kotlin/Java so this repo probably isn't a shining light of code quality anyway lol! If @myrjola has time to review you might get some better feedback but I'll see what I can do!

@joerdav
Copy link
Collaborator

joerdav commented Jun 26, 2024

This looks good, seems that the main feature is implementing the new rawgo feature. I'll do some manual testing then merge.

@chabad360
Copy link
Author

Don't merge just yet, this isn't quite done (tho I will try and wrap it up later today). I just needed a second pair of eyes to review.

@chabad360
Copy link
Author

Additionally, it seems I maybe should have a look at https://plugins.jetbrains.com/docs/intellij/implementing-lexer.html#embedded-language. It probably would cover at least some of the crazy tricks I'm pulling.

@chabad360
Copy link
Author

Okay, this is at a point where I need more eyes on it. I need some help figuring out how to add tests for the GO and HTML PSI files. I need people to find where this breaks, I don't use templ enough to really find that (I used it a drop recently and the experience was poor enough that I'm working on this now).

@chabad360
Copy link
Author

chabad360 commented Jun 26, 2024

There are still a bunch of certain errors from Go Inspections, if anyone has any idea how to appease those, please lmk.

@bastianwegge
Copy link

@chabad360 can you compile a "latest" version and upload the zip? Here myrjola did the same so we could test his version.

@chabad360
Copy link
Author

chabad360 commented Jun 27, 2024

Templ-0.0.14.zip

Here you go

@chabad360
Copy link
Author

@bastianwegge I just realized I uploaded an old version, here's the correct one:

Templ-0.0.14.1.zip

@bastianwegge
Copy link

bastianwegge commented Jun 27, 2024

/e: Will probably come back later and try again 👍

Here's the first badge of feedback, I'll come back later with more as I'm on the train and have to leave now 😅 . All regarding the version you posted 1 hour ago.

  1. The package statement doesn't seem to work correctly
package
  1. Component Children (for template composition) dont work
children
  1. I get "unused import" for every package I import

@bastianwegge
Copy link

Just one thing that popped out right away (next feedback) is that inline go code (printing a string) is recognized as an error. I like the recognition of the templ-Keyword, the ComponentName and expressions, really good job!

inline-go-code

@chabad360
Copy link
Author

chabad360 commented Jun 28, 2024

Templ-0.0.14.2.zip

@bastianwegge fixed that issue.

really good job!

Thanks. I'm actually not entirely sure why highlighting doesn't work in more areas, trying to figure it out.

@bastianwegge
Copy link

Here's the next badge of feedback:

  1. I played around with this piece of code:
package main

type Option struct {
	Value string
	Label string
}

type Config struct {
	Options *[]Option
}

templ Select(c *Config) {
	<div>
		<select>
			for _, option := range *c.Options {
				<option value={ option.Value }>{ option.Label }</option>
			}
		</select>
	</div>
}

templ SimpleSelectUsage() {
	@Select(&Config{
		Options: &[]Option{
			{Value: "1", Label: "One"},
			{Value: "2", Label: "Two"},
			{Value: "3", Label: "Three"},
			{Value: "4", Label: "Four"},
		},
	})
}

It shows the following errors for me:

slice:
error-slice-multiline

options:
error-options

comma:
error-comma

brace:
error-brace

  1. variadics don't seem to be recognized right now
variadic-2 variadic-1

@myrjola
Copy link
Contributor

myrjola commented Jun 29, 2024

Great progress! Thanks for taking a stab at this ❤️

I didn't know it was possible to add support for multiple languages through the TemplateLanguageFileViewProvider. Your approach looks promising. Let's squash the remaining bugs.

How did you come up with this approach? Are there any existing plugins you're taking inspiration from? They might be helpful in figuring out the missing parts.

A testing tip that was helpful for me was to try the plugin against all the templ files in the Templ repository. There's many examples inside the generator directory.

Here's some issues I encountered besides the ones reported above. I'm afraid I won't have time to dive into the rabbit hole to understand the issues better but I hope they can be fixed before merging these changes.

Multiple notNullChild crashes in Go plugin

I'm getting crashes any time there's string expressions in the templ component.

java.lang.IllegalStateException: @NotNull method com/intellij/psi/impl/PsiElementBase.notNullChild must not return null
	at com.intellij.psi.impl.PsiElementBase.$$$reportNull$$$0(PsiElementBase.java)
	at com.intellij.psi.impl.PsiElementBase.notNullChild(PsiElementBase.java:282)
	at com.goide.psi.impl.GoVarDefinitionImpl.getIdentifier(GoVarDefinitionImpl.java:39)

I believe this is caused by the Go parser's PSI tree not populating identifiers for the string expressions. I can see that we're prepending var _ string = before the expression to make it a proper Go statement.

                TemplTypes.GO_EXPR -> {
                    if (baseLexer.state == _TemplLexer.IN_EXPR) {
                        modifications.addRangeToRemove(baseLexer.tokenStart, "var _ string = ")
                        modifications.addAll(
                            this.appendCurrentTemplateToken(baseLexer.tokenEnd, baseLexer.tokenSequence)
                        )
                    }

Notice that the VAR_DEFINITION is empty in the PSI Viewer

image

But not when I write a similar statement in the raw Go code outside templ components.

image

I don't know what's causing this. I tried working around it by creating a Go statement by wrapping the string expression with print( and ). Unfortunately, it gives a very similar notNullChild error but this time for the print identifier being empty.

Could be interesting to see if it behaves the same with MultiHostInjector.

Do you encounter this notNullChild crash in your testing?

Autoimport corrupts the file

When I autocomplete new libraries to a Templ file like this:

image

The package and import statements sometime become corrupted

image

I'm suspecting this is caused by adding the import "github.com/a-h/templ" in GoLanguageType. I'm not sure how to approach this. The Templ LSP is already doing autoimports as of a-h/templ#138, maybe there's some way to disable autoimports in the Go plugin and delegate them to the LSP?

@chabad360
Copy link
Author

chabad360 commented Jun 30, 2024

How did you come up with this approach?

Brute force.

A testing tip that was helpful for me was to try the plugin against all the templ files in the Templ repository. There's many examples inside the generator directory.

This is a good idea. Honestly tho, it would be a lot more useful if I had some sort of tooling that could show me what's being sent to the Go PSI and HTML PSI and show me errors that it throws (rn I need to set some specific breakpoints and have only one file open when I start the debug session, it's annoying).

Multiple notNullChild crashes in Go plugin

Ah yes, that's been bugging me and I haven't bothered enough yet to pursue it, so thanks for tracking it down.

Could be interesting to see if it behaves the same with MultiHostInjector.

Hmm, this approach certainly seems like it might be a bit saner than the way I'm doing things currently, although, I'm not entirely certain it can actually do everything I need it to...

I'm suspecting this is caused by adding the import "github.com/a-h/templ" in GoLanguageType. I'm not sure how to approach this. The Templ LSP is already doing autoimports as of a-h/templ#138, maybe there's some way to disable autoimports in the Go plugin and delegate them to the LSP?

I was worried about that. I can probably make a quick fix that prevents the corruption (change the lexer to include the new line), but this would still be an issue. It's probably possible to disable Go plugin auto-imports, but I'll need to do some digging to figure it out.

@chabad360
Copy link
Author

chabad360 commented Jun 30, 2024

Templ-0.0.14.3.zip

@bastianwegge

  1. I played around with this piece of code:

I think I have fixed the bugs here.

  1. variadics don't seem to be recognized right now

Hrm, I only specifically blocked children... because it's a special case. I'm not sure what to do about attrs because it's a param that can be named whatever one wants and that's a bit of a minefield (there might be some shenanigans that I can do with state tracking). In general tho, varidics are a bit of a complicated case, I may just block them for now and hope that doesn't break anything.

EDIT: Figured out the specific issue you ran into, and blocked Go from seeing varidics in general.

@myrjola

Autoimport corrupts the file

I'm not able to test that I actually fixed this atm, but if you can give it a try on your end and let me know if it's corrupting the file that would be great.

@chabad360
Copy link
Author

Templ-0.0.14.4.zip

Fixed a regression.

@up1io
Copy link

up1io commented Jul 24, 2024

Is there a roadmap/plan for releasing this as a new version?

@chabad360
Copy link
Author

chabad360 commented Jul 24, 2024

@up1io I think that's up to the maintainers ultimately. But I would like to finish this up. I got a bit busy with work and some other stuff recently. But I will get some more stuff done here between this week and next week.

@up1io
Copy link

up1io commented Jul 25, 2024

Okay, thanks @chabad360 for the update and thanks x10 for your efforts. Your latest build is a real improvement for me.

@chabad360
Copy link
Author

@myrjola

Could be interesting to see if it behaves the same with MultiHostInjector.

So I did try, however, I'm running against the limits of my java knowledge, and tbh, I'm not quite sure how to fix this as the code examples they give don't give any hints (XmlText doesn't look like it implements PsiLanguageInjectionHost either).

package com.templ.templ.injector;

import com.goide.GoLanguage;
import com.intellij.lang.injection.MultiHostInjector;
import com.intellij.lang.injection.MultiHostRegistrar;
import com.intellij.openapi.util.TextRange;
import com.intellij.psi.PsiElement;
import com.intellij.psi.PsiLanguageInjectionHost;
import com.templ.templ.psi.TemplExpr;
import com.templ.templ.psi.TemplTypes;
import org.jetbrains.annotations.NotNull;

import java.util.List;

final class GolangToTemplInjector implements MultiHostInjector {
    @Override
    public void getLanguagesToInject(@NotNull MultiHostRegistrar registrar, @NotNull PsiElement context) {
        if (context instanceof TemplExpr) {
            registrar.startInjecting(GoLanguage.INSTANCE);

            registrar.addPlace("var _ string =", ";", (PsiLanguageInjectionHost)context, getBodyRange(context));

            registrar.doneInjecting();
        }
    }

    private TextRange getBodyRange(@NotNull PsiElement context) {
        return context.getChildren()[1].getTextRange();
    }

    @Override
    public @NotNull List<? extends Class<? extends PsiElement>> elementsToInjectIn() {
        return List.of(TemplExpr.class);
    }
}

Results in a very long exception that starts:

java.lang.ClassCastException: class com.templ.templ.psi.impl.TemplExprImpl cannot be cast to class com.intellij.psi.PsiLanguageInjectionHost (com.templ.templ.psi.impl.TemplExprImpl is in unnamed module of loader com.intellij.ide.plugins.cl.PluginClassLoader @23762d80; com.intellij.psi.PsiLanguageInjectionHost is in unnamed module of loader com.intellij.util.lang.PathClassLoader @1888ff2c)
	at com.templ.templ.injector.GolangToTemplInjector.getLanguagesToInject(GolangToTemplInjector.java:21)

I think I'm going to assume there's a bug somewhere in-between the Go Parser and the template injector. I'm gonna see if I can track it down (but honestly it's a bit hard to figure out where exactly it gets passed to the Go parser) and file a bug/figure out a workaround.

@joerdav joerdav mentioned this pull request Aug 20, 2024
@alexisvisco
Copy link

@chabad360 any news ? :)

@chabad360
Copy link
Author

Ha! No.

I'm travelling at the moment, but when I'm back home I'm gonna try again against the latest intellij, and if the bug is still present I'm just gonna write up a bug report (which I should have done months ago).

@WhosLogan
Copy link

Any news on this?

@AlexVasiluta
Copy link

Hello! Any updates?

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

Successfully merging this pull request may close these issues.

Add more token colours

8 participants