Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Syntax Definition Push and Immediately Pop (both non-consuming patterns) Bug #1190

Closed
keith-hall opened this issue Apr 30, 2016 · 5 comments
Closed

Comments

@keith-hall
Copy link
Collaborator

Summary

There is a bug with syntax definitions that push two or more contexts onto the stack without consuming any characters, and then immediately pop the top one back off again also without consuming any characters. It causes ST3 to skip a character.

Expected behavior

The 2nd context should be processed at the same character at the point the multiple contexts were pushed.

Actual behavior

The 2nd context's match patterns are processed after one character has been skipped.

Steps to reproduce

  1. Create a sublime-syntax file that includes contexts and patterns as per the summary, something like:

    %YAML 1.2
    ---
    # See http://www.sublimetext.com/docs/3/syntax.html
    scope: source.example-push-pop-bug
    contexts:
      main:
        - match: 'test'
          scope: constant.numeric
          push: c
      a:
        - meta_scope: test
        - match: 'hello'
          scope: string.unquoted
        - match: 'ello'
          scope: keyword.control
      b:
        - match: (?=.)
          pop: true
      c:
        - match: '(?=h)'
          push: [a, b]
    
  2. Create a new tab, assign it this newly created syntax

  3. Type text that will meet the requirements, something like:

    testhello
    
  4. Notice that one character is skipped for pattern matching - in this case, the h - despite having the correct meta scope

push pop non consuming bug
push pop non consuming bug2

Environment

Windows 7 x64, Ubuntu 16.04 x64
Untested on a Mac

ST3 build 3111

@keith-hall
Copy link
Collaborator Author

Just to clarify, empty match patterns - match: '' also count as not consuming any characters - it doesn't just occur with lookaheads.

Interestingly, this slightly modified variant of the sublime-syntax shows that the pushed context refuses to be popped until a character is consumed (either by a match pattern or just skipping the character):

%YAML 1.2
---
# See http://www.sublimetext.com/docs/3/syntax.html
scope: source.example-push-pop-bug
contexts:
  main:
    - match: 'test'
      scope: constant.numeric
      push: c
  a:
    - meta_content_scope: test
    - match: 'h'
      scope: string.unquoted
    - match: 'ello'
      scope: keyword.control
  b:
    - match: ''
      pop: true
    - match: '(?=.)'
      pop: true
    - match: '(?=h)'
      pop: true
    - match: 'h'
      scope: entity.name.function
    - match: 'e'
      scope: storage.type
  c:
    - match: ''
      push: [a, b]

push pop non consuming bug3
push pop non consuming bug4

and while parsing the sublime-syntax file, there is a line in the console:

rule #anon_string_2 has a scope name, but is unreachable, so the name will never be used

@keith-hall
Copy link
Collaborator Author

It makes sense to force a character to be consumed when pushing a single context onto the stack, so that infinite push/pop recursion doesn't occur, but this logic doesn't hold true when pushing multiple contexts. Maybe it could be changed to move the check onto the first pushed context (remembering that it is first-in-last-out) instead of the top most / last pushed context?

@FichteFoll
Copy link
Collaborator

The syntax highlighter already has a check for infinite loops (or at least until a certain threshold), which displays an error message upon triggering and should indicate a bad syntax definition since such a thing should not be possible, so I believe that the safeguard would not be necessary even for single context pushes.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Dec 17, 2016

I also ran into this today. Very annoying because it limits some of the theoretically achievable functionality. There is a workaround for my use case, but is quite ugly and I'd rather not use it (I would need it three times, even).

FichteFoll added a commit to FichteForks/Packages that referenced this issue Dec 19, 2016
... and use it to match function calls less explicitly and more
magically. Usually you would think this is bad, but it's not because
it's not really less explicit. It just avoids repetition when scoping
the last (non-special) part of a qualified name with a name other than
"meta.generic-name".

This change triggers the bug described in
sublimehq/sublime_text#1190, where the
most-inner context in a multi-push operation *must* match at least one
character before it can be popped.

Due this, the tests are currently *not* passing.
@wbond wbond added this to the Build 3127 milestone Jan 11, 2017
@wbond wbond added the R: fixed label Jan 11, 2017
@wbond
Copy link
Member

wbond commented Apr 12, 2017

This was addressed in build 3127

@wbond wbond closed this as completed Apr 12, 2017
jcberquist added a commit to jcberquist/sublimetext-cfml that referenced this issue Aug 14, 2017
Function declaration matching was failing on 3126 due to
sublimehq/sublime_text#1190
deathaxe pushed a commit to deathaxe/sublime-packages that referenced this issue Jun 9, 2019
... and use it to match function calls less explicitly and more
magically. Usually you would think this is bad, but it's not because
it's not really less explicit. It just avoids repetition when scoping
the last (non-special) part of a qualified name with a name other than
"meta.generic-name".

This change triggers the bug described in
sublimehq/sublime_text#1190, where the
most-inner context in a multi-push operation *must* match at least one
character before it can be popped.

Due this, the tests are currently *not* passing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants