clear_scopes behaves differently with push/set

[deathaxe]

This issue may already be part of #2326 - not sure, but it adds a minimal test case to illustrate how clear_scopes: 1 behaves differently when using push or set. The attached test file fails at exactly one position.

The example scopes a function definition in two different ways. Functions starting with @ push the context to scope the parameter list to the stack while functions starting with $ set the context to stack to do the same.

As each function does only have one parameter list, using set makes somehow more sense as we want to pop off the param-... context as soon as the closing ) is matched and continue with the next one (which is meta in the example but could be an arbitrary list of contexts in real world).

Actual Behavior

clear_scope: 1 does not clear the meta.function.test scope from the opening (. Hence it contains both meta.function.test and meta.function.parameters.test.

The meta.function.test is already cleared from the closing ) for both push and set.

Expected Behavior

clear_scope: 1 should clear the meta.function.test scope from the opening ( so it contains only meta.function.parameters.test. This is the behavior when using push already. Both set and push should have the same result.

ClearScope.sublime-syntax

%YAML 1.2
---
# See http://www.sublimetext.com/docs/3/syntax.html
file_extensions:
  - test
scope: source.test
contexts:
  main:
    - match: \@\w+
      scope: entity.name.function.test
      push:
        - meta
        - param-push

    - match: \$\w+
      scope: entity.name.function.test
      push:
        - meta
        - param-set

  param-push:
    - match: \(
      scope: punctuation.section.parameters.begin.test
      push:
        - clear_scopes: 1
        - meta_scope: meta.function.parameters.push.test
        - match: \)
          scope: punctuation.section.parameters.end.test
          pop: true
    - match: $|(?=\S)
      pop: true

  param-set:
    - match: \(
      scope: punctuation.section.parameters.begin.test
      set:
        - clear_scopes: 1
        - meta_scope: meta.function.parameters.set.test
        - match: \)
          scope: punctuation.section.parameters.end.test
          pop: true
    - match: $|(?=\S)
      pop: true

  meta:
    - meta_scope: meta.function.test
    - match: ''
      pop: true

Syntax Test

# SYNTAX TEST "ClearScope.sublime-syntax"

@fun(par)
#^^^ meta.function.test - meta.function.parameters.push.test
#   ^^^^^ meta.function.parameters.push.test - meta.function.test

$fun(par)
#^^^ meta.function.test - meta.function.parameters.set.test
#   ^^^^^ meta.function.parameters.set.test - meta.function.test

Console

Packages/User/syntax_test_clearscope.test:7:5: [meta.function.parameters.set.test - meta.function.test] does not match scope [source.test meta.function.test meta.function.parameters.set.test punctuation.section.parameters.begin.test]
FAILED: 1 of 16 assertions in 1 files failed
[Finished]

[wbond]

I was working on addressing this. After changes and running the tests on the default packages, all 193 assertion failures are from rules written by you @deathaxe 😃.

[deathaxe]

Propably yes, as I tried to work around or work with this behavior to avoid overlapping meta scopes. I'd be keen enough to have a look at those when the time has come ;-)

[deathaxe]

I guess Erlang contains some of the 193 failures?

Example:

  preproc-control-definitions:
    - match: (?=\()
      set:
        - clear_scopes: 1
        - match: \(
          scope: punctuation.section.arguments.begin.erlang
          set:
            - - clear_scopes: 1
              - meta_scope: meta.preprocessor.conditional.arguments.erlang
              - include: expect-arguments-end
            - constant-other-macro
    - include: preproc-stray-arguments-end

With (the second) clear_scope working properly the (?=\() lookahead could be removed.

[wbond]

Yes, I was mostly just amused that the only situations where this bug would have been hit (in the default packages) were written by you, and you submitted the issue.

Clearly it is mostly just a style of how you approach transitioning between contexts, and there certainly isn't anything wrong with that. It is definitely something that should be fixed, IMO, while we are in the process of making breaking changes.

[FichteFoll]

I'd expect PackageDev to also have failures, if you want to test that as well. Actually not, I'm not using clear_scopes in two adjacent contexts like in the example above.

A similar pattern is used in Python but without clear_scopes since the meta scope is set in the context that matches the identifier. I would generally prefer the approach here, if it worked properly, so I'm happy for the fix. It should simplify some of the meta.mapping context logic as well.

[wbond]

This has been fixed in build 4075, but only for version: 2 of .sublime-syntax. This is due to the fact that changing it for all syntaxes would have broken backwards compatibility.