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

Fall back to 'Plain Text' if a referenced syntax is missing #427

Merged
merged 2 commits into from
Mar 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/parsing/syntax_definition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,18 @@ pub enum ContextReference {
ByScope {
scope: Scope,
sub_context: Option<String>,
/// `true` if this reference by scope is part of an `embed` for which
/// there is an `escape`. In other words a reference for a context for
/// which there "always is a way out". Enables falling back to `Plain
/// Text` syntax in case the referenced scope is missing.
with_escape: bool,
},
#[non_exhaustive]
File {
name: String,
sub_context: Option<String>,
/// Same semantics as for [`Self::ByScope::with_escape`].
with_escape: bool,
},
#[non_exhaustive]
Inline(String),
Expand Down
96 changes: 89 additions & 7 deletions src/parsing/syntax_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -749,15 +749,15 @@ impl SyntaxSetBuilder {
all_context_ids[syntax_index].get(s)
}
}
ByScope { scope, ref sub_context } => {
Self::find_id(sub_context, all_context_ids, syntaxes, |index_and_syntax| {
ByScope { scope, ref sub_context, with_escape } => {
Self::with_plain_text_fallback(all_context_ids, syntaxes, with_escape, Self::find_id(sub_context, all_context_ids, syntaxes, |index_and_syntax| {
index_and_syntax.1.scope == scope
})
}))
}
File { ref name, ref sub_context } => {
Self::find_id(sub_context, all_context_ids, syntaxes, |index_and_syntax| {
File { ref name, ref sub_context, with_escape } => {
Self::with_plain_text_fallback(all_context_ids, syntaxes, with_escape, Self::find_id(sub_context, all_context_ids, syntaxes, |index_and_syntax| {
&index_and_syntax.1.name == name
})
}))
}
Direct(_) => None,
};
Expand All @@ -767,6 +767,32 @@ impl SyntaxSetBuilder {
}
}

fn with_plain_text_fallback<'a>(
Copy link
Collaborator Author

@Enselic Enselic Mar 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried putting this logic inside of Self::find_id() directly, which would simplify the code. But rustc complained about reached the recursion limit while instantiating 'func::<[closure]>'. This is because the predicate parameter is generic, and calling find_id() inside of itself causes recursion without a base case. This problem is explained in detail here.

This was the most straightforward way I could come up with to work around that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering about that, thanks for the explanation :)

all_context_ids: &'a [HashMap<String, ContextId>],
syntaxes: &'a [SyntaxReference],
with_escape: bool,
context_id: Option<&'a ContextId>,
) -> Option<&'a ContextId> {
context_id.or_else(|| {
if with_escape {
// If we keep this reference unresolved, syntect will crash
// when it encounters the reference. Rather than crashing,
// we instead fall back to "Plain Text". This seems to be
// how Sublime Text behaves. It should be a safe thing to do
// since `embed`s always includes an `escape` to get out of
// the `embed`.
Self::find_id(
&None,
all_context_ids,
syntaxes,
|index_and_syntax| index_and_syntax.1.name == "Plain Text",
)
} else {
None
}
})
}

fn find_id<'a>(
sub_context: &Option<String>,
all_context_ids: &'a [HashMap<String, ContextId>],
Expand Down Expand Up @@ -954,6 +980,62 @@ mod tests {
assert_ops_contain(&ops, &expected);
}

#[test]
fn falls_back_to_plain_text_when_embedded_scope_is_missing() {
test_plain_text_fallback(r#"
name: Z
scope: source.z
file_extensions: [z]
contexts:
main:
- match: 'z'
scope: z
- match: 'go_x'
embed: scope:does.not.exist
escape: 'leave_x'
"#);
}

#[test]
fn falls_back_to_plain_text_when_embedded_file_is_missing() {
test_plain_text_fallback(r#"
name: Z
scope: source.z
file_extensions: [z]
contexts:
main:
- match: 'z'
scope: z
- match: 'go_x'
embed: DoesNotExist.sublime-syntax
escape: 'leave_x'
"#);
}

fn test_plain_text_fallback(syntax_definition: &str) {
let syntax =
SyntaxDefinition::load_from_str(syntax_definition, true, None).unwrap();

let mut builder = SyntaxSetBuilder::new();
builder.add_plain_text_syntax();
builder.add(syntax);
let syntax_set = builder.build();

let syntax = syntax_set.find_syntax_by_extension("z").unwrap();
let mut parse_state = ParseState::new(syntax);
let ops = parse_state.parse_line("z go_x x leave_x z", &syntax_set);
let expected_ops = vec![
(0, ScopeStackOp::Push(Scope::new("source.z").unwrap())),
(0, ScopeStackOp::Push(Scope::new("z").unwrap())),
(1, ScopeStackOp::Pop(1)),
(6, ScopeStackOp::Push(Scope::new("text.plain").unwrap())),
(9, ScopeStackOp::Pop(1)),
(17, ScopeStackOp::Push(Scope::new("z").unwrap())),
(18, ScopeStackOp::Pop(1)),
];
assert_eq!(ops, expected_ops);
}

#[test]
fn can_find_unlinked_contexts() {
let syntax_set = {
Expand All @@ -974,7 +1056,7 @@ mod tests {

let unlinked_contexts : Vec<String> = syntax_set.find_unlinked_contexts().into_iter().collect();
assert_eq!(unlinked_contexts.len(), 1);
assert_eq!(unlinked_contexts[0], "Syntax 'A' with scope 'source.a' has unresolved context reference ByScope { scope: <source.b>, sub_context: Some(\"main\") }");
assert_eq!(unlinked_contexts[0], "Syntax 'A' with scope 'source.a' has unresolved context reference ByScope { scope: <source.b>, sub_context: Some(\"main\"), with_escape: false }");
}

#[test]
Expand Down
41 changes: 33 additions & 8 deletions src/parsing/yaml_load.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ impl SyntaxDefinition {
if !is_special {
if let Ok(x) = get_key(map, "include", Some) {
let reference = SyntaxDefinition::parse_reference(
x, state, contexts, namer)?;
x, state, contexts, namer, false)?;
context.patterns.push(Pattern::Include(reference));
} else {
let pattern = SyntaxDefinition::parse_match_pattern(
Expand All @@ -228,7 +228,8 @@ impl SyntaxDefinition {
fn parse_reference(y: &Yaml,
state: &mut ParserState<'_>,
contexts: &mut HashMap<String, Context>,
namer: &mut ContextNamer)
namer: &mut ContextNamer,
with_escape: bool)
-> Result<ContextReference, ParseSyntaxError> {
if let Some(s) = y.as_str() {
let parts: Vec<&str> = s.split('#').collect();
Expand All @@ -243,6 +244,7 @@ impl SyntaxDefinition {
.build(&parts[0][6..])
.map_err(ParseSyntaxError::InvalidScope)?,
sub_context,
with_escape,
})
} else if parts[0].ends_with(".sublime-syntax") {
let stem = Path::new(parts[0])
Expand All @@ -252,6 +254,7 @@ impl SyntaxDefinition {
Ok(ContextReference::File {
name: stem.to_owned(),
sub_context,
with_escape,
})
} else {
Ok(ContextReference::Named(parts[0].to_owned()))
Expand Down Expand Up @@ -320,7 +323,7 @@ impl SyntaxDefinition {
namer,
)?;
MatchOperation::Push(vec![ContextReference::Inline(escape_context),
SyntaxDefinition::parse_reference(y, state, contexts, namer)?])
SyntaxDefinition::parse_reference(y, state, contexts, namer, true)?])
} else {
return Err(ParseSyntaxError::MissingMandatoryKey("escape"));
}
Expand Down Expand Up @@ -375,10 +378,10 @@ impl SyntaxDefinition {
y.as_vec()
.unwrap()
.iter()
.map(|x| SyntaxDefinition::parse_reference(x, state, contexts, namer))
.map(|x| SyntaxDefinition::parse_reference(x, state, contexts, namer, false))
.collect()
} else {
let reference = SyntaxDefinition::parse_reference(y, state, contexts, namer)?;
let reference = SyntaxDefinition::parse_reference(y, state, contexts, namer, false)?;
Ok(vec![reference])
}
}
Expand Down Expand Up @@ -912,10 +915,15 @@ mod tests {
// this is sadly necessary because Context is not Eq because of the Regex
let expected = MatchOperation::Push(vec![
Named("string".to_owned()),
ByScope { scope: Scope::new("source.c").unwrap(), sub_context: Some("main".to_owned()) },
ByScope {
scope: Scope::new("source.c").unwrap(),
sub_context: Some("main".to_owned()),
with_escape: false,
},
File {
name: "CSS".to_owned(),
sub_context: Some("rule-list-body".to_owned())
sub_context: Some("rule-list-body".to_owned()),
with_escape: false,
},
]);
assert_eq!(format!("{:?}", match_pat.operation),
Expand Down Expand Up @@ -952,7 +960,7 @@ mod tests {
pop: true
"#,false, None).unwrap();

let def_with_embed = SyntaxDefinition::load_from_str(r#"
let mut def_with_embed = SyntaxDefinition::load_from_str(r#"
name: C
scope: source.c
file_extensions: [c, h]
Expand All @@ -968,6 +976,23 @@ mod tests {
escape: (?i)(?=</style)
"#,false, None).unwrap();

// We will soon do an `assert_eq!()`. But there is one difference we must expect, namely
// that for `def_with_embed`, the value of `ContextReference::ByScope::with_escape` will be
// `true`, whereas for `old_def` it will be `false`. So manually adjust `with_escape` to
// `false` so that `assert_eq!()` will work.
let def_with_embed_context = def_with_embed.contexts.get_mut("main").unwrap();
if let Pattern::Match(ref mut match_pattern) = def_with_embed_context.patterns[0] {
if let MatchOperation::Push(ref mut context_references) = match_pattern.operation {
if let ContextReference::ByScope {
ref mut with_escape,
..
} = context_references[1]
{
*with_escape = false;
}
}
}

assert_eq!(old_def.contexts["main"], def_with_embed.contexts["main"]);
}

Expand Down