Follow-ups after adding `default.yaml` support #3259

dmaluka · 2024-04-18T22:25:17Z

More follow-ups of #2933.

Fix set filetype unknown not working as expected in some scenarios.
Allow includes in default.yaml.
There is still even more stuff to fix and to refactor in the existing UpdateRules() logic. Just added TODOs for now.

Fix `set filetype unknown` not working as expected in the following scenario: 1. open foo.txt (no filetype detected) -> ft is `unknown`, highlighted with default.yaml, as expected 2. `set filetype go` -> ft is `go`, highlighted with go.yaml as expected 3. `set filetype unknown` -> ft is still `go`, still highlighted with go.yaml (whereas expected behavior is: ft is `unknown`, highlighted with default.yaml) Fix that by always updating b.SyntaxDef value, not reusing the old one. This also makes the code simpler and easier to understand.

The "runtime" term is ambiguous: it refers to both built-in and user's custom ("real runtime") files.

JoeKar · 2024-04-19T16:19:24Z

internal/buffer/buffer.go

@@ -690,6 +690,7 @@ func parseDefFromFile(f config.RuntimeFile, header *highlight.Header) *highlight
 	}

 	if header == nil {
+		// TODO: use faster highlight.MakeHeader() when possible


But highlight.MakeHeader() can only be used, when we iterate over config.RTSyntaxHeader, otherwise we can't guarantee, that we access the correct lines or am I wrong?
So in case of findRuntimeSyntaxDef() we could iterate over the config.RTSyntaxHeader first, to create the header structure and then iterate over the normal config.RTSyntax and passing the already created header to parseDefFromFile().

Yes, so my point was that we should try to use config.RTSyntaxHeader whenever possible, to make it faster. That's what it is for.

Actually later I've realized that this should not have much impact on performance (for now): in findRuntimeSyntaxDef("default", nil) we're gonna call this MakeHeaderYaml() just once (for the file that satisfies the condition f.Name() == "default"), not hundreds of times.

So I think I'll remove this TODO. All this still looks messy, but I guess we should rather start cleaning this mess by figuring out how to fix the buggy include logic (per below comments) without making the code unreadable again...

JoeKar · 2024-04-19T16:21:41Z

internal/buffer/buffer.go

 	if b.SyntaxDef != nil && highlight.HasIncludes(b.SyntaxDef) {
 		includes := highlight.GetIncludes(b.SyntaxDef)

 		var files []*highlight.File
+		// TODO: search in the user's custom syntax files first


Yep, user defined includes aren't considered yet, but are worth to be considered.
It wasn't in the scope of my PR.

Yeah, this issue is separate from your PR, this mess has been here before.

Maybe the following patch is a point to start...
RefactorIncludesInUpdateRules.patch.txt
...it's not yet tested, but maybe you can improve and extend it or you throw it away and create your own approach. 😉

It shall consider nested includes, as long as the include isn't present in the list.

Open TODOs:

prevent cyclic include

process user files as includes

It shall consider nested includes

Hmm yeah, good point, thanks.

Though it seems your patch will still not fix it: highlight.ResolveIncludes() is still gonna handle the current file only.

Looks like ParseDef() itself would need to take care about includes, transparently?

Though it seems your patch will still not fix it: highlight.ResolveIncludes() is still gonna handle the current file only.

Hm, the problem is that...

func resolveIncludesInDef(files []*File, d *Def) { for _, lang := range d.rules.includes { [...] } [...] }

...in resolveIncludesInDef(), which again targets the base syntax definition and his level of includes only. Looks like we could drop this outer loop. Then it will append the (unfortunately) once again parsed rules to the base syntax definition, what I would expect it to do...since we already collected all files involved.

Edit1:
The above mentioned doesn't seem to be that easy, since the regions are processed in the same way.

After looking once again at my updated version of parseDefFromFile() (three returns) I came to the conclusion, that this isn't the best approach, since every iterated file will be fully parsed (all steps), even when we don't need it. So I will return to the single parsing steps within the searchSyntaxDefIncludes() till we've a better way to optimize the duplication there.

Looks like ParseDef() itself would need to take care about includes, transparently?

No bad idea, to fully keep it out of the buffer. But then we need to find a way how to deal with the different runtime files, controlled and feed by the buffer.

Edit2:
Ok, since ParseDef() will be involved already twice with the rework it doesn't seem to be feasible. A better logic, than already prepared, seems to be necessary.
Furthermore the logic of the buffer asking the highlighter for includes, searching in the runtime files and give them the highlighter back to do some additional resolving is complicated and hard to follow.

Edit3:
RefactorIncludesInUpdateRules2.patch.txt
Not really convincing. :(

Edit4:

Looks like ParseDef() itself would need to take care about includes, transparently?

No bad idea, to fully keep it out of the buffer. But then we need to find a way how to deal with the different runtime files, controlled and feed by the buffer.

Seems definitely to be the best idea and we don't need to care any longer about this jumping forth into the highlighter, back to the buffer and forth into the highlighter again. Let us do this directly while parsing the file respective definition of it. The rules and regions can be build and append in one step.
👍

...I was wrong: we already ensure that in the list returned by config.ListRuntimeFiles() user's custom files always come before default files. Moreover, since your #3066 we ensure that the same filetype never appears twice: if there are both a default file and a user's custom file for the given filetype, this list only contains the user's custom one. So we don't need to fix anything in this regard.

...Regarding nested includes: yes, jumping back and forth between the buffer and the highlighter seems to be a no-go, it would be tremendously ugly and would be just a work around the broken design of the highlight parser's API. The way to go seems to be to redesign this API.

But I don't feel like redesigning it right now, so maybe let's just document that nested includes are not supported for now.

RefactorIncludesInUpdateRules2.patch.txt
Not really convincing. :(

Actually the basic idea of your patch, i.e. just move the bulk of the include logic to a separate function and then fix it there, seems reasonable. Although, as I've just found, there is basically nothing to fix, but still it seems a good idea to move it to a separate function, to make UpdateRules() even shorter.

internal/buffer/buffer.go

dmaluka · 2024-04-21T12:51:39Z

internal/buffer/buffer.go

 	if b.SyntaxDef != nil && highlight.HasIncludes(b.SyntaxDef) {
 		includes := highlight.GetIncludes(b.SyntaxDef)

 		var files []*highlight.File
+		// TODO: search in the user's custom syntax files first


...I was wrong: we already ensure that in the list returned by config.ListRuntimeFiles() user's custom files always come before default files. Moreover, since your #3066 we ensure that the same filetype never appears twice: if there are both a default file and a user's custom file for the given filetype, this list only contains the user's custom one. So we don't need to fix anything in this regard.

...Regarding nested includes: yes, jumping back and forth between the buffer and the highlighter seems to be a no-go, it would be tremendously ugly and would be just a work around the broken design of the highlight parser's API. The way to go seems to be to redesign this API.

But I don't feel like redesigning it right now, so maybe let's just document that nested includes are not supported for now.

RefactorIncludesInUpdateRules2.patch.txt
Not really convincing. :(

Actually the basic idea of your patch, i.e. just move the bulk of the include logic to a separate function and then fix it there, seems reasonable. Although, as I've just found, there is basically nothing to fix, but still it seems a good idea to move it to a separate function, to make UpdateRules() even shorter.

dmaluka · 2024-04-21T12:54:38Z

internal/buffer/buffer.go

@@ -950,6 +952,8 @@ func (b *Buffer) UpdateRules() {
 					break
 				}
 			}
+			// TODO: fix this mess. For each include, we should include
+			// exactly one file: either a built-in one or a user's custom one


So, I was wrong about this one too: since #3066 we already ensure that exactly one file will be included.

This len(files) >= len(includes) check is now not strictly necessary, but it provides a simple optimization (to exit the loop earlier), so we can keep it.

dmaluka added 3 commits April 18, 2024 22:39

UpdateRules: allow includes in default.yaml

a436dae

UpdateRules: correct the comments

3aed20f

The "runtime" term is ambiguous: it refers to both built-in and user's custom ("real runtime") files.

JoeKar reviewed Apr 19, 2024

View reviewed changes

dmaluka commented Apr 21, 2024

View reviewed changes

dmaluka added 2 commits April 21, 2024 15:13

UpdateRules: move include logic to a helper function

1bddc8d

UpdateRules: optimize out HasIncludes() usage

08c516c

dmaluka force-pushed the default-syntax-followup branch from eb81277 to 08c516c Compare April 21, 2024 13:16

help/colors: syntax: document that nested includes are not supported

b929c61

JoeKar approved these changes Apr 21, 2024

View reviewed changes

dmaluka merged commit 169a9a6 into zyedidia:master Apr 21, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow-ups after adding `default.yaml` support #3259

Follow-ups after adding `default.yaml` support #3259

dmaluka commented Apr 18, 2024

JoeKar Apr 19, 2024

dmaluka Apr 19, 2024

JoeKar Apr 19, 2024

dmaluka Apr 19, 2024

JoeKar Apr 19, 2024

dmaluka Apr 19, 2024

JoeKar Apr 20, 2024 •

edited

Loading

dmaluka Apr 21, 2024

dmaluka Apr 21, 2024

dmaluka Apr 21, 2024

Follow-ups after adding default.yaml support #3259

Follow-ups after adding default.yaml support #3259

Conversation

dmaluka commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeKar Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Follow-ups after adding `default.yaml` support #3259

Follow-ups after adding `default.yaml` support #3259

JoeKar Apr 20, 2024 •

edited

Loading