metaborg / nabl

Spoofax' Name Binding Language
Apache License 2.0
7 stars 12 forks source link

Allow escaped characters in templates (messages) #90

Closed MeAmAnUsername closed 2 years ago

MeAmAnUsername commented 2 years ago

Allow escaped characters (\n, \t, \r) in templates (i.e. notes, warnings and errors). Also adds some syntax tests to test this. Tested with my own project, it seems to work.

I did not change any code generation, so if there is any code generation then it should be checked if that still works correctly.

Resolves #83

AZWN commented 2 years ago

Thanks for your contribution! It looks neat. Before merging, I have a question: Did you test whether messages with special characters are properly escaped (<br> etc.) in Eclipse?

MeAmAnUsername commented 2 years ago

They are not. How/where should that be done?

MeAmAnUsername commented 2 years ago

I see two solutions, and two locations for each solution to be used, for a total of 4 options:

Solution 1: wrap the current TermFormatter in one that also escapes HTML TextPart#toString

    @Override public String toString(@SuppressWarnings("unused") TermFormatter formatter) {
        return text;
    }

It currently does not use the term formatter, but instead returns the string unmodified. If it used the term formatter, and the term formatter escaped HTML characters, that would work. I'd prefer to basically just wrap the existing one in one that then also escapes HTML characters, e.g. termFormatter = new escapeHTMLFormatter(new UnifierFormatter(...))

Option 1: wrap in Solver#shallowTermFormatter Doesn't seem like a good idea, because that ties the Solver to Eclipse.

Option 2: Wrap in SpoofaxPrimitive#formatMessage The mb.statix.spoofax package seems to be the boundary layer between Statix and Spoofax. This means it will also escape if Spoofax is run on the command line / with Gradle, but that cannot be solved in this repository. ~I consider this the best solution if I'm the one that has to implement it.~ edit: not anymore

Solution 2: escape after formatting the message

_Option 3: escape in STX_extract_messages_ Escape HTML characters just before providing them to Spoofax via Stratego primitive. Could be in STX_extract_messages itself or in the helper method StatixPrimitive#addMessage.

Option 4: escape in Spoofax Escape the actual message strings at the position where they are wrapped in Eclipse messages. That does not happen in this repository but in "the" Spoofax Eclipse repository, wherever that may be. In my personal opinion Statix and the other meta-DSLs should not be concerned about encoding the messages for Eclipse, so this might well be the cleanest solution. However, I don't have the slightest clue where this should be done, so I will not be implementing this option.

Note Escaping for newlines and such at the boundary from Statix message templates to Java Strings is already implemented in StatixTerms#unescapeMessageText.

MeAmAnUsername commented 2 years ago

I currently have three questions that need to be answered before I continue on this:

  1. Do you have comments, opinions or preferences for the options above?
  2. How do we want to escape the HTML characters? 2.1. Using an existing library, e.g. Apache Commons. Easy but pulls in a new dependency. 2.2. Roll my own: escape all characters except ASCII 2.3. Roll my own: escape only \n, \t and \r 2.4. Use whatever is already used (see question 3)
  3. I just realized that some characters are already escaped (e.g. > and non-breaking space). Do you know where that happens?
AZWN commented 2 years ago

Option 3 was the way to go :-). Thanx for the contribution!