included contexts should still be able to pop using backrefs #104
Comments
What do you mean by "
shiiiiiit. If I remember correctly, I look for pops with back-references as an optimization so that I don't have to save the regions when that isn't necessary. I'll probably have to fix this to look deeper using the normal context iteration logic. I'm not sure how tricky this will be but it might be tricky. |
I've made some tests and it seems that in ST, This can be observed in ST with the following syntax: %YAML 1.2
---
# See http://www.sublimetext.com/docs/3/syntax.html
scope: test.with_prototype
contexts:
main:
- match: 'hello'
scope: string
push: other1
with_prototype:
- match: 'other1?'
scope: comment
- match: '\d+'
scope: constant.numeric
- match: 'bar'
scope: punctuation.separator
other1:
- match: 'world'
scope: string
push: other2
with_prototype:
- match: '(an)?other2?'
scope: storage.type
- match: '\d+'
scope: keyword
- match: 'something_else'
scope: entity.name.prototypes
- match: '(?=bar)'
pop: true
other2:
- match: 'foo'
scope: invalid.deprecated
pop: truethe idea behind this example syntax is that it should push two contexts - both using a different So if we have sample text
This shows that the patterns from the first pushed |
|
@keith-hall I thought I fixed that discrepancy in #103 with d0b253c. Which version are you testing with? |
I'm testing with the latest version, 1.7.2. I created a syntax test file for my example above and tested and it does indeed pass in syntect, so maybe I'm wrong about the problem. (Sorry, should have checked that earlier but didn't get chance.)But I'm confused because the failures for ASP show that it is scoping |
|
The issue of popping with backrefs also applies to |
Fails at runtime due to RefCell already mutably borrowed errors
|
So I figured out why this happens, but it's tricky to fix, and even trickier to fix properly. Basically the actual regex compiling logic handles this case fine, but the captures don't get saved since the context that only indirectly includes patterns with backrefs isn't marked as needing backrefs. The proper way to fix this is after linking, walk over the tree and mark contexts as needing backrefs if any context they include needs backrefs, but I don't want to write another tree pass right now. I attempted a fix in https://github.com/trishume/syntect/tree/fix-104 but it fails with Do you know if any syntax actually uses this behaviour? |
|
The PackageDev ST package uses it in it's TextMate Preferences syntax definition, but it might not be used in the official ST Packages repo atm. |
|
It might be useful to benchmark how much difference it makes whether we just always store the captures vs performing the context uses backref check (i.e. https://github.com/forkeith/syntect/tree/104_include_backref_pop_fix) |
|
|
I ran the benches on https://github.com/forkeith/syntect/tree/104_include_backref_pop_fix and I don't see any significant differences with the current master |
|
I ran the new fancy Criterion benchmarks on the change of removing the optimization and there's a substantial performance regression: And with #167 I note that at least with the current master Packages there's no problems caused by this optimization. So for now I'll leave this open and not remove the optimization, pending the fix I mention in a comment above involving a new tree walk. |
|
Now that all the contexts are in a flat arena, I wonder if this would be easier to achieve. A test case that could be used in the parser (maybe it would also make sense to have a test in the syntax_set, directly checking the #[test]
fn can_include_backrefs() {
let syntax = SyntaxDefinition::load_from_str(r#"
name: Backref Include Test
scope: source.backrefinc
contexts:
main:
- match: (a)
scope: a
push: context1
context1:
- include: context2
context2:
- match: \1
scope: b
pop: true
"#, true, None).unwrap();
expect_scope_stacks_with_syntax(&"aa", &["<a>", "<b>"], syntax);
} |
Now that #101 is fixed, I was able to get the ASP syntax tests to pass by changing the syntax definition to not include a
with_prototypeconflict - namely by changing [1] and [2] fromscope:text.html.asp#htmltoscope:text.html.basic:Emboldened by that, I decided to try some more syntaxes out (SublimeText/PackageDev#98), but found a bug. In ST, it is always possible for a match pattern that will
popto use back references captured from the original push pattern, regardless of whether that pattern is a direct child of the context that was pushed, or was included from it at any nesting. In syntect, it only seems to allow back references that refer to the original push pattern if the pop pattern is a direct child of the context that was pushed onto the stack.In syntect, an included context trying to access the back references currently causes a panic (please expand the arrow to the left of this paragraph for the backtrace).
I've managed to put a small example syntax together that exhibits this behavior:
which can be used with the following syntax test:
The text was updated successfully, but these errors were encountered: