JuliaEditorSupport / julia-intellij

:computer: Julia Plugin for IntelliJ IDEA ┗:smiley:┛ ┏:smiley:┓ ┗:smiley:┛
GNU General Public License v3.0
303 stars 30 forks source link

Converting ordinary to compact overloaded functions does not work properly #457

Open mattBrzezinski opened 4 years ago

mattBrzezinski commented 4 years ago

If you have an overloaded function such as:

Base.isapprox(ad::Wirtinger)
    # some code here
end

Converting this to a compact function results in:

isapprox(ad::Wirtinger) =  # some code here

Converting this back to an ordinary function results in:

Base.function isapprox(ad::Wirtinger)
    # some code here
end

However, converting from the block above results in the proper compact version of:

Base.isapprox(ad::Wirtinger) =  # some code here

Properly this should just go back and forth between:

function Base.isapprox(ad::Wirtinger)
   # some code here
end

Base.isapprox(ad::Wirtinger) =  # some code here
mattBrzezinski commented 4 years ago

Labels should probably be bug and parser.

mattBrzezinski commented 4 years ago

@ice1000 I haven't worked with Kotlin, could you point me to where these changes need to be made?

I found the compact function and the function function. I assume we want to be changing the nameIdentifier here?

Also what are the easiest steps for testing changes? Rebuild the plugin, load it up and test? Or is there a quicker way?


EDIT: Just figure out testing can be done w/ gradle buildPlugin && gradle runIde

ice1000 commented 4 years ago

Also what are the easiest steps for testing changes? Rebuild the plugin, load it up and test? Or is there a quicker way?

gradlew runIde will start an IDE with the latest plugin compiled, hope it's helpful


EDIT: Just saw your EDIT

ice1000 commented 4 years ago

You wanna update this code:

https://github.com/JuliaEditorSupport/julia-intellij/blob/bb466164605744e19329176666987d7383d7cd58/src/org/ice1000/julia/lang/editing/julia-annotator.kt#L104

https://github.com/JuliaEditorSupport/julia-intellij/blob/bb466164605744e19329176666987d7383d7cd58/src/org/ice1000/julia/lang/editing/julia-annotator.kt#L117

mattBrzezinski commented 4 years ago

Spent some more time working on this and trying to learn more of Kotlin. An example function that I'm playing with is:

function Base.length()
    return 0
end

The PSI Tree for is:

FILE(0,39)
  JuliaStatementsImpl(STATEMENTS)(0,39)
    JuliaFunctionImpl(FUNCTION)(0,39)
      PsiElement(FUNCTION_KEYWORD)('function')(0,8)
      PsiElement(SYM)('Base')(9,13)
      PsiElement(DOT_SYM)('.')(13,14)
      JuliaSymbolImpl(SYMBOL)(14,20)
        PsiElement(SYM)('length')(14,20)
      JuliaFunctionSignatureImpl(FUNCTION_SIGNATURE)(20,22)
        PsiElement(LEFT_BRACKET)('(')(20,21)
        PsiElement(RIGHT_BRACKET)(')')(21,22)
      PsiElement(EOL)('\n')(22,23)
      JuliaStatementsImpl(STATEMENTS)(27,36)
        JuliaReturnExprImpl(RETURN_EXPR)(27,35)
          PsiElement(RETURN_KEYWORD)('return')(27,33)
          JuliaIntegerImpl(INTEGER)(34,35)
            PsiElement(INT_LITERAL)('0')(34,35)
        PsiElement(EOL)('\n')(35,36)
      PsiElement(END_KEYWORD)('end')(36,39)

I cannot seem to be able to get access to the PSIElement Base here, I've tried element.findElementAt(N) but it always returns back function regardless of N. I've also tried accessing it via children but that returns back the entire object itself.

Not sure how these elements are created but would we not always want to store the nameIdentifier as the string following function? i.e. just combine the following (if SYM and DOT_SYM are present).

PsiElement(SYM)('Base')(9,13)
PsiElement(DOT_SYM)('.')(13,14)
JuliaSymbolImpl(SYMBOL)(14,20)
ice1000 commented 4 years ago

Have you tried using the combination of .firstChild/.nextSibling?

ice1000 commented 4 years ago

Fuck!! I think I need to give this project a big refactoring. I got a lot of better practices on doing many things, and this project keeps using oldschool stupid way. Like, .children.filter { bla } can be rewritten as .childrenWithLeaves.filter { bla } given these utilities:

https://github.com/pest-parser/intellij-pest/blob/71f524c65cb688bdb4efec7ee17a6116e81c54f4/src/rs/pest/psi/utils.kt#L10-L14

And that's gonna be a lazy stream operation.

ice1000 commented 4 years ago

Also .children gives you an array with non-leaf PsiElements, which is the problem you're getting. .childrenWithLeaves gives you leaves so that's what you need

ice1000 commented 4 years ago

And mostly we're filtering .children's return value as well so why not use .childrenWithLeaves in the very beginning

ice1000 commented 4 years ago

Man I just don't wanna open this project in my IDE. Why am I being lazy like this

ice1000 commented 4 years ago

I'm in a bad mood

ice1000 commented 4 years ago

I talk too much

mattBrzezinski commented 4 years ago

Thanks! I should be able to have this issue wrapped up in the next while, got the element I needed now :)

mattBrzezinski commented 4 years ago

Ignore the poor naming, this is my WIP commit:

https://github.com/mattBrzezinski/julia-intellij/commit/589bcf58f61441b9e342a8d84d900df3e30c4c4c

This seems like it should work, however when it does the replace it just deletes the function it's attempting to replace. Any ideas on how to debug this?

ice1000 commented 4 years ago

Any ideas on how to debug this?

Possible cause: your generated julia code doesn't parse

mattBrzezinski commented 4 years ago

Oops forgot to include my example code to convert:

function Base.length()
  return 0
end

Base.length() = return 0

function length()
  return 0
end

With your comment addressed: https://github.com/JuliaEditorSupport/julia-intellij/commit/2a3ace65281606246b9268710080c2c177566df3

ice1000 commented 4 years ago

I'm having classes, will reply later

mattBrzezinski commented 4 years ago

Any thoughts? Played around with it a bit more, but seem stuck.

ice1000 commented 4 years ago

Have you tried

ice1000 commented 4 years ago

Do you know how to use the debugger to evaluate an expression?

ice1000 commented 4 years ago

Could you check the output of JuliaTokenType.fromText(<the string you passed to `JuliaReplaceWithTextIntention`>)? If it's null, you're producing the wrong string

mattBrzezinski commented 4 years ago

I tried following the guide for setting up the Debugger here however it seems out of date.

ice1000 commented 4 years ago

Btw, kids is a very lovely name. I like it, it's not poor at all

ice1000 commented 4 years ago

I tried following the guide for setting up the Debugger here however it seems out of date.

I mean, debug the plugin

mattBrzezinski commented 4 years ago

I've just been using gradle runIde to verify my changes.

ice1000 commented 4 years ago

Set a breakpoint in the IDE, right click "runIde" task in the gradle window and select "debug"

Then you'll be able to view the local variables while the IDE is running

ice1000 commented 4 years ago

You can evaluate expressions during debugging

ice1000 commented 4 years ago

Like this image

ice1000 commented 4 years ago

Speaking of element.findElementAt(N), you shouldn't use it. You should find the DOT token, and if it exists, return the token before it (.prevSibling). This would be more robust because you may have functions that aren't extension functions

ice1000 commented 4 years ago

And if the .prevSibling is a whitespace token, continue to find its prevSibling

ice1000 commented 4 years ago

I got many utilities for things like this in other projects such as intellij-pest or intellij-dtlc, feel free to copy them to the utils.kt files

mattBrzezinski commented 4 years ago

Got the debugger setup, the value is always what is expected. Either length or Base.length depending on which function is being analyzed.

mattBrzezinski commented 4 years ago

Function I am trying to compact:

function Base.length()
  return 0
end

Screenshot_2020-01-15 10 45 39_D2xS3y

ice1000 commented 4 years ago

Could you check the output of JuliaTokenType.fromText(<the string you passed to `JuliaReplaceWithTextIntention`>)? If it's null, you're producing the wrong string

Did you follow this?

mattBrzezinski commented 4 years ago

This is from JuliaReplaceWithTextIntention()

new = "Base.length () = 0"
result = JuliaTokenType.fromText(new, project)  # PsiWhiteSpace

Inside fromText()

createFileFromText(JuliaLanguage.INSTANCE, code) . code: " Base.length () = 0"
mattBrzezinski commented 4 years ago

Ok got this working.... I had to change:

val values = kids.joinToString(separator="") { it.text }

To

val values = kids.joinToString(separator="") { it.text }.trim()

And it works how it's supposed to.

EDIT: I lied. It only works one way, from ordinary to compact. The opposite fails because of the PsiTree. I'll keep poking away at this, thanks for the help :)

ice1000 commented 4 years ago

Yay! (From someone who still don't wanna open his IDE)

mattBrzezinski commented 4 years ago

Draft CR

Last piece to this is removing the overloaded function from being prepended to function when converting from a compact to ordinary function. I cannot seem to find where this is being set.

ice1000 commented 4 years ago

PsiElement has a delete method. You may want your own JulaBlablablaQuickFix class if you need

mattBrzezinski commented 4 years ago

Sorry, I'm not following. Currently with the proposed CR above when you have:

Base.length() = 0

It will return:

Base.function Base.length()
  return 0
end

I cannot find where Base.function is being set. I can trace it back to fromText, and everything seems fine. But after the break point on this function it just prepends Base. to function.

ice1000 commented 4 years ago

No

ice1000 commented 4 years ago

length() = 0 is parsed as a function, Base. is a prefix out of the function

ice1000 commented 4 years ago

I believe that's the reason why you're getting the result

ice1000 commented 4 years ago

Or maybe you've deleted the wrong PsiElement, so length() = 0 gets replaced with a function blabla thing and the prefix Base. is kept

mattBrzezinski commented 4 years ago

Looking at the PsiTree for the above example. There are three elements to it (denoted with square brackets).

[Base][.][length() = return 0]

When doing the replace I'm changing the length() = return 0. Everything else is getting thrown away, with this call. I can't find where the prefix is being set however.

ice1000 commented 4 years ago

I meant "prefix" by the combination of Base and ., two tokens

ice1000 commented 4 years ago

The first argument to the constructor of JuliaReplaceWithTextIntention is the PsiElement gets replaced by the action. You can change it from element to something else

mattBrzezinski commented 4 years ago

Yeah, I can't find where this is being set when converting from compact to an ordinary function.

https://github.com/JuliaEditorSupport/julia-intellij/blob/master/src/org/ice1000/julia/lang/editing/julia-annotator.kt#L84

Here we're just writing function .... but I'm not understanding where Base. is prepended to function?

ice1000 commented 4 years ago

Here we're just writing function .... but I'm not understanding where Base. is prepended to function?

YOU ARE ON THE WRONG TRACK

ice1000 commented 4 years ago

The JuliaReplaceWithTextIntention does two things:

  1. What you see is Base.length() = 0, but what gets passed to the compactFunction method is length() = 0
  2. Delete the first argument in the editor, which is a PsiElement, which is length() = 0
  3. Insert function ... thing

So you get Base.function ...