Fix for atom/language-xml#91 and atom/language-xml#96

[vfcp]

#96 already provides all the details about the issues fixed here.

#87 (comment) has the correct code but merge included some extra indent which causes the rule not to work properly.

In relation with #91, a begin without end or while was added but this is not valid as begin should always have a corresponding end or while. match should be used instead of begin

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• All new code requires tests to ensure against regressions

Description of the Change

The details are already discussed in detail in both issues #91 and #96.

Alternate Designs

N/A

Benefits

Fix invalid rules for JSP style comments, fixes #91

Possible Drawbacks

None

Applicable Issues

#91 #96

[Ingramz]

Regardless of whether the changes here stay the same, I think it would be good overall if we had a couple of extra tests to better explain what kind of output we expect from snippets containing -- in comments. Right now the test only concerns the happy path where there exists --> after <!-- ... -- ..., but now we might also need to include some cases that also include highlightable code after -->; case where --> does not exist and the edge case including --->. It was somewhat (but not entirely) fine to omit these earlier, as the ouput was roughtly the same for anything past --, now they are different.

We can wait till the proposal in the corresponding issue gets finalized and then look at which tests exactly have to be implemented so that the tests do not need to be redone multiple times.

[darangi]

@vfcp could you add some tests?

[vfcp]

I've added a test for <!-- invalid -- comment ---><n></n>--><n></n> which together with the already existing tests cover all cases mentioned by @Ingramz above.

Below is the test and the list of tokens generated for your review:

  it 'tokenizes after invalid comment only if comment was properly closed', ->
    {tokens} = grammar.tokenizeLine('<!-- invalid -- comment ---><n></n>--><n></n>')
    expect(tokens[0]).toEqual value: '<!--', scopes: ['text.xml', 'comment.block.xml', 'punctuation.definition.comment.xml']
    expect(tokens[1]).toEqual value: ' invalid ', scopes: ['text.xml', 'comment.block.xml']
    expect(tokens[2]).toEqual value: '--', scopes: ['text.xml', 'comment.block.xml', 'invalid.illegal.bad-comments-or-CDATA.xml']
    expect(tokens[3]).toEqual value: ' comment -', scopes: ['text.xml', 'comment.block.xml', 'invalid.illegal.bad-comments-or-CDATA.xml']
    expect(tokens[4]).toEqual value: '--', scopes: ['text.xml', 'comment.block.xml', 'invalid.illegal.bad-comments-or-CDATA.xml']
    expect(tokens[5]).toEqual value: '><n></n>', scopes: ['text.xml', 'comment.block.xml', 'invalid.illegal.bad-comments-or-CDATA.xml']
    expect(tokens[6]).toEqual value: '-->', scopes: ['text.xml', 'comment.block.xml', 'punctuation.definition.comment.xml']
    expect(tokens[7]).toEqual value: '<',   scopes: ['text.xml', 'meta.tag.no-content.xml', 'punctuation.definition.tag.xml']
    expect(tokens[8]).toEqual value: 'n',   scopes: ['text.xml', 'meta.tag.no-content.xml', 'entity.name.tag.xml', 'entity.name.tag.localname.xml']
    expect(tokens[9]).toEqual value: '>',   scopes: ['text.xml', 'meta.tag.no-content.xml', 'punctuation.definition.tag.xml']
    expect(tokens[10]).toEqual value: '</', scopes: ['text.xml', 'meta.tag.no-content.xml', 'punctuation.definition.tag.xml']
    expect(tokens[11]).toEqual value: 'n',  scopes: ['text.xml', 'meta.tag.no-content.xml', 'entity.name.tag.xml', 'entity.name.tag.localname.xml']
    expect(tokens[12]).toEqual value: '>',  scopes: ['text.xml', 'meta.tag.no-content.xml', 'punctuation.definition.tag.xml']

[Ingramz]

The tests seem sufficient for now, however I (and probably also you) noticed that becuase of the (?=-->) it stops everwhere inside the invalid comment where --> is encountered and starts a new token. Would it be fine for you to change that matcher to (?<!-)(?=-->) and update tests so that contiguous parts on single line get matched as one large token instead of many small ones?

[vfcp]

While testing your suggestion to use (?<!-)(?=-->) I've noticed that some of the changes we made caused empty XML comments like <!----> not to be parsed correctly.

To summarize, this is the current rule. It's basically the same rule as before PR with addiction of 'end': '(?<!-)(?=-->)' to break invalid comment scope in case proper --> is found:

      {
        'begin': '<!--'
        'captures':
          '0':
            'name': 'punctuation.definition.comment.xml'
        'end': '-->'
        'name': 'comment.block.xml'
        'patterns': [
          {
            'begin': '--(?!>)'
            'end': '(?<!-)(?=-->)'
            'name': 'invalid.illegal.bad-comments-or-CDATA.xml'
          }
        ]
      }

List of tokens produced:

  it 'tokenizes after invalid comment only if comment was properly closed', ->
    {tokens} = grammar.tokenizeLine('<!-- invalid -- comment ---><n></n>--><n></n>')
    expect(tokens[0]).toEqual value: '<!--', scopes: ['text.xml', 'comment.block.xml', 'punctuation.definition.comment.xml']
    expect(tokens[1]).toEqual value: ' invalid ', scopes: ['text.xml', 'comment.block.xml']
    expect(tokens[2]).toEqual value: '--', scopes: ['text.xml', 'comment.block.xml', 'invalid.illegal.bad-comments-or-CDATA.xml']
    expect(tokens[3]).toEqual value: ' comment ---><n></n>', scopes: ['text.xml', 'comment.block.xml', 'invalid.illegal.bad-comments-or-CDATA.xml']
    expect(tokens[4]).toEqual value: '-->', scopes: ['text.xml', 'comment.block.xml', 'punctuation.definition.comment.xml']
    expect(tokens[5]).toEqual value: '<',   scopes: ['text.xml', 'meta.tag.no-content.xml', 'punctuation.definition.tag.xml']
    expect(tokens[6]).toEqual value: 'n',   scopes: ['text.xml', 'meta.tag.no-content.xml', 'entity.name.tag.xml', 'entity.name.tag.localname.xml']
    expect(tokens[7]).toEqual value: '>',   scopes: ['text.xml', 'meta.tag.no-content.xml', 'punctuation.definition.tag.xml']
    expect(tokens[8]).toEqual value: '</', scopes: ['text.xml', 'meta.tag.no-content.xml', 'punctuation.definition.tag.xml']
    expect(tokens[9]).toEqual value: 'n',  scopes: ['text.xml', 'meta.tag.no-content.xml', 'entity.name.tag.xml', 'entity.name.tag.localname.xml']
    expect(tokens[10]).toEqual value: '>',  scopes: ['text.xml', 'meta.tag.no-content.xml', 'punctuation.definition.tag.xml']

I've also added an additional test for the <!----> scenario which might be useful in future changes.

[Ingramz]

Thank you for pointing this out, I'm glad at least we got one extra test out of it. I know for certain that we can do better, but at this point I'm not sure how easy it would be. We have made it clear enough that it's unintentional that multiple tokens are generated, but also realize that nobody should be impacted at least right now (file an issue report or a pull request if you are). The test suite should support now enough cases to not break it in a dumb manner in the future.

[Ingramz]

@vfcp actually I think this is simpler than I thought before, we could move --(?!>) into a lookahead: (?=--(?!>)), that should always create at most one token.

[vfcp]

@Ingramz Updated rule and tests as suggested.

I've tested in Linguist and VSCode as well to make sure behavior is same as expected.

[Ingramz]

Excellent!

@darangi it should be now good to review/merge.