Removal of error reporting markers from parameter types

[Wonseo-C]

The string returned by getTargetType included error reporting markers, which it shouldn't.

Before this change:

After this change:

[cmnrd]

I don't know the specifics of the TS code generation, but simply using the type id does not seem like the right solution to me. What if the type is time? Or time[]? It seems that getTargetType() does not do exactly what is needed for TS here.

[hokeun]

but simply using the type id does not seem like the right solution to me.

I agree. @Wonseo-C, how about debugging to figure out why getTargetType returns a wrong value here? Ideally, we would like to fix the getTargetType function rather than just changing it. Also, does this only happen to the runner? Or does this happen to other .lf programs as well?

[Wonseo-C]

@cmnrd @hokeun
I tried debugging getTargetType and found that there is some bug.
The function returns parameter.inferredType's toText().
So, there is some bug with toText()'s return value.

Also, I found that when I used the toOriginalText(), I got the expected return value "number".

How can I fix this problem?

[cmnrd]

@petervdonovan could you have a look at this? Is it save to just use toOriginalText() here? It looks like the code tagging is breaking things in TypeScript.

[Wonseo-C]

@petervdonovan could you have a look at this? Is it save to just use toOriginalText() here? It looks like the code tagging is breaking things in TypeScript.

I changed getTargetType()'s return value toOriginalText()

[petervdonovan]

Sorry for missing @cmnrd's comment.

Since I don't know the full motivation behind this change, it is difficult for me to comment on whether it is necessary. It can cause slight regressions in the error reporting functionality of the language server, which needs these tags in order to report errors on the correct lines in VS Code.

A possible alternative would be to strip the CodeMap tags out downstream, as is done here using CodeMap.fromGeneratedCode(someToTextOutputWithTags).generatedCode. This might not be the most elegant solution, but I think it would be the right choice if the result of getTargetType is only wrong for a few specific cases. (I mean, if it is fine for the code generators, but not what we want when using it for something else.)

[lhstrh]

@petervdonovan: the problem is that the type of the parameter instead of returning something like string or number would return a much longer string with additional markers in it used for error reporting. I think this is a legitimate bug, and the text referenced in the method should be the original one, not the modified one. I'm not quite sure how this fix would affect error reporting...

Are you suggesting that we replace the line that says type.toOriginalText() with CodeMap.fromGeneratedCode(type).generatedCode? If you could make a concrete code suggestion in the Files changed, that would be great.

[Wonseo-C]

Are you suggesting that we replace the line that says type.toOriginalText() with CodeMap.fromGeneratedCode(type).generatedCode? If you could make a concrete code suggestion in the Files changed, that would be great.

@lhstrh Does this mean that we'll use CodeMap.fromGeneratedCode(type).generatedCode like under the screenshot?
I did debug like this, and then I got the value of "number\n", not "number".

And sorry for the bad readability. I added the code link in the main comment..!

[petervdonovan]

Sorry for the late response...

Are you suggesting that we replace the line that says type.toOriginalText() with CodeMap.fromGeneratedCode(type).generatedCode? If you could make a concrete code suggestion in the Files changed, that would be great.

That is not quite what I meant. If we did that, it would be equivalent to just using toOriginalText, except that it is more complicated because it puts the CodeMap.Correspondence tags in and then takes them out again.

I cannot suggest a change in the Files changed because my suggestion applied only to the code that originally motivated this PR. I do not know why you wanted to change getTargetType, but I guessed that it was because somewhere else in the code base, the CodeMap.Correspondence tags were causing problems. If the tags are causing problems elsewhere, then the change I suggested would be a workaround.

I think this is a legitimate bug, and the text referenced in the method should be the original one, not the modified one.

I'm not sure why this would be the case. The philosophy behind my liberal use of these tags is that the primary purpose of the functions that convert EObjects to text is for code generation, and that all generated code copied verbatim from the source should have a mapping. To access the contents of the EObject, we can just directly call its getters, right? It strikes me as slightly hacky to use the functions related to code generation to stringify these objects, and then use regexes and string functions to analyze them -- sure, we do it, and I probably have done it too, but it isn't something we should aspire to because it is a heuristic-based approach that creates weird bugs and corner cases.

That said, this has been a lot of discussion over a relatively harmless change. I am happy to see this PR get merged if that's necessary in order to make progress.

[lhstrh]

I guess the real issue is that the markers should associate not with a parameter type but with a declaration that uses it... I agree that it would be reasonable to merge this in order to make progress... This can always be revised at a later time.