allow using not registered highlighting groups

[andrejlevkovitch]

PR Prelude

Thank you for working on YCM! :)

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your PR:

• I have read and understood YCM's CONTRIBUTING document.
• I have read and understood YCM's CODE_OF_CONDUCT document.
• I have included tests for the changes in my PR. If not, I have included a
  rationale for why I haven't.
• I understand my PR may be closed if it becomes obvious I didn't
  actually perform all of these steps.

Why this change is necessary and useful

Clangd supports some non-standard highlighting groups, for example label, so currently we will see warning in vim when C/C++ code contains any labels. And even if we manually set text property for label (like YCM_HL_label) it will not fix that, because YCM will ignore any not registered groups. So, instead, I add checking the "new" group in text properties - if it is already there, then add it to the list of registered groups

PS unfortunately I am not a good at python and I didn't look deep in that repo, so I didn't provide test case for that, only checked it in editor with label group

---

This change is 

[puremourning]

Thanks for sending a PR!

Just so I understand, could you please clarify what issue this is resolving? I'm not 100% clear from the description.

According to the docs (https://github.com/ycm-core/YouCompleteMe#customising-the-highlight-groups) you should be able to deinfe a prop_type for custom syntax tokens:

Some servers also use custom values. In this case, YCM prints a warning including the token type name that you can customise.

I was fairly sure that worked, but if you're suggesting it doesn't could you provide steps to reproduce the issue so that I can at least test/verify it manually?

[codecov]

Codecov Report

Merging #4161 (f041d0d) into master (142a559) will decrease coverage by 10.52%.
The diff coverage is 100.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4161       +/-   ##
===========================================
- Coverage   89.02%   78.50%   -10.52%     
===========================================
  Files          34       34               
  Lines        4390     4396        +6     
===========================================
- Hits         3908     3451      -457     
- Misses        482      945      +463     

[andrejlevkovitch]

According to the docs you should be able to deinfe a prop_type for custom syntax tokens

You can define that property, but it will not be used by YCM right now

Some servers also use custom values. In this case, YCM prints a warning including the token type name that you can customise.

YCM prints the warning even if property defined. It is because checking of properties happens by searching them in registered highlighting groups

could you provide steps to reproduce the issue so that I can at least test/verify it manually?

You can check it easily by adding highlighting group for label (for example Label, so in vim it should be in yellow). After that check this piece of code:

#include <cstdio>
int foo(int val) {
  if (val) {
    printf("%d\n", val);
  } else {
    goto Skip;
  }

  return 1;

Skip:
  return 0;
}

With current version of YCM you will see that warning (mentioned above), but Skip will not be colored. But if you apply my pr, then you will not see the warning and Skip will be colored

PS also use latest clang, as I'm not sure in what version label was added

[puremourning]

I cannot reproduce with your example.

https://asciinema.org/a/1tMEhT9Bfkxxk4nErmntqSuo4

While I'm happy to believe there might be a bug here, I can't really merge a change without a clear repro even if manual.

[puremourning]

PS also use latest clang, as I'm not sure in what version label was added

I'm using the supported version (the one that ships with YCM; 16.0.1). Are you using a custom version?

[andrejlevkovitch]

I'm using the supported version (the one that ships with YCM; 16.0.1). Are you using a custom version?

I use latest one, 17.0.0. I think 16.0.1 doesn't have label group. Anyway, you still can reproduce issue with 16.0.1, but for that you need to remove one of groups from HIGHLIGHT_GROUP dictionary. For example namespace. All other steps should be the same

[andrejlevkovitch]

@puremourning do you able to check that with clangd-17 or by removing one of HIGHLIGHT_GROUP?

[puremourning]

I am building llvm main branch right now.

[andrejlevkovitch]

@puremourning I hope you build only clangd, not entire llvm :)

cmake -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DCMAKE_BUILD_TYPE=Release ../llvm
cmake --build . --target clangd

[puremourning]

OK thanks, I can repro now with main clangd.

I think really we should just add label to the list, as clangd is a supported engine (though the released version 16.0.2 does not have this issue).

I did a quick patch locally like this, seems better?

ben@BenMBP2021 YouCompleteMe % git diff --cached
diff --git a/python/ycm/semantic_highlighting.py b/python/ycm/semantic_highlighting.py
index b96c28fb..b6f48d02 100644
--- a/python/ycm/semantic_highlighting.py
+++ b/python/ycm/semantic_highlighting.py
@@ -22,6 +22,8 @@ from ycm import vimsupport
 from ycm import text_properties as tp
 from ycm import scrolling_range as sr

+import vim
+

 HIGHLIGHT_GROUP = {
   'namespace': 'Type',
@@ -107,16 +109,21 @@ class SemanticHighlighting( sr.ScrollingBufferRange ):
     self._prop_id = NextPropID()

     for token in tokens:
-      if token[ 'type' ] not in HIGHLIGHT_GROUP:
-        if token[ 'type' ] not in REPORTED_MISSING_TYPES:
-          REPORTED_MISSING_TYPES.add( token[ 'type' ] )
-          vimsupport.PostVimMessage(
-            f"Missing property type for { token[ 'type' ] }" )
-        continue
       prop_type = f"YCM_HL_{ token[ 'type' ] }"
-
       rng = token[ 'range' ]
       self.GrowRangeIfNeeded( rng )
-      tp.AddTextProperty( self._bufnr, self._prop_id, prop_type, rng )
+
+      try:
+        tp.AddTextProperty( self._bufnr, self._prop_id, prop_type, rng )
+      except vim.error as e:
+        if 'E971:' in str( e ): # Text property doesn't exist
+          if token[ 'type' ] not in REPORTED_MISSING_TYPES:
+            REPORTED_MISSING_TYPES.add( token[ 'type' ] )
+            vimsupport.PostVimMessage(
+              f"Token type { token[ 'type' ] } not supported. "
+              f"Define property type { prop_type }. "
+              f"See :help youcompleteme-customising-highlight-groups" )
+        else:
+          raise e

     tp.ClearTextProperties( self._bufnr, prop_id = prev_prop_id )

wdyt?

[andrejlevkovitch]

Are you sure that your patch is optimal solution? I am not sure that usage of vim exceptions is nice and fast way. I think that it adds additional not necessary overhead. But maybe I'm wrong.

[puremourning]

@puremourning I hope you build only clangd, not entire llvm :)

cmake -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DCMAKE_BUILD_TYPE=Release ../llvm
cmake --build . --target clangd

yes, it's not my first rodeo :)

[puremourning]

Are you sure that your patch is optimal solution? I am not sure that usage of vim exceptions is nice and fast way. I think that it adds additional not necessary overhead. But maybe I'm wrong.

The problem with your way (and the way I initially thought to implement it too) is that it doesn't respond to changes at runtime.

The message says "define text prop XXX" and you define it with call prop_type_add( 'XXX', ... ) but it still doesn't work. This way it works. It's kinda pythonic to do EAFTP too, though that's not a great reason to do it this way. I mean, you may be right that in the case where there are a lot of these, it could be bad for perf but in practice it should be relatively small, and user can fix it with a single line in vimrc or a single :call command. So I think I'm ok with it.

[puremourning]

Thanks again for helping out !