Packages: [C] indexing broken in 3.1 for functions with newline after return type

[jsbmsu]

Copied from forum post about the same issue:

I just updated to the new ST3 release (build 3170) today, and some C functions aren't getting indexed and highlighted correctly anymore. Here is a simple snippet that reproduces the problem:

LIB_RESULT
foo()
{
   return LIB_SUCCESS;
}

LIB_RESULT bar()
{
    return LIB_SUCCESS;
}

In this example, with Monokai in use, foo is highlighted in blue, and bar is highlighted in green. Only 'bar' is actually indexed; i.e., the Goto symbol palette only lists bar, not foo. This behavior is a regression from build 3143.

With some experimentation, it appears that if the return type is at least 3 characters long and consists only of capital letters, numerals, and underscores, then the problem reproduces. I assume that someone was trying to produce a heuristic to filter out extra qualifiers like calling convention, inline, etc., which may be obfuscated through preprocessor macros (which are conventionally UPPER_SNAKE), but it has the side effect of breaking for perfectly valid return types. UEFI and WINAPI both fall in this bucket; surely they are not alone.

It would be nice if this could be made to just work. If not, then perhaps there could be a workaround in which the user specifies a list of known types in some preferences override, so that they are treated as return types and not mere decorations. Does anyone know if ctags would get me around this, if I can manage to generate them?

[rwols]

I assume that someone was trying to produce a heuristic to filter out extra qualifiers

That would be me :-) I think it might be this one: https://github.com/sublimehq/Packages/pull/1037. Note also the corresponding issue: https://github.com/sublimehq/Packages/issues/1034

The nice thing about this heuristic is that it circumvents preprocessor macro constructs like this:

#define FOOLIB_NAMESPACE_BEGIN namespace foo {
#define FOOLIB_NAMESPACE_END }

FOOLIB_NAMESPACE_BEGIN

class Foo {};

FOOLIB_NAMESPACE_END

And it also fixes a bunch of files like this one or this one.

perhaps there could be a workaround in which the user specifies a list of known types in some preferences override, so that they are treated as return types and not mere decorations.

This would be nice, but not possible currently.

Perhaps we could disable the heuristic for C and keep things as-is for C++? Not entirely convinced of myself, but it's a start perhaps.

[jsbmsu]

perhaps there could be a workaround in which the user specifies a list of known types in some preferences override, so that they are treated as return types and not mere decorations.

This would be nice, but not possible currently.

Could you shed any light on why it's not possible? E.g., is there no plumbing for parser preferences? Is the C/C++ parser not designed in a way that would be conducive to an exception list?

Perhaps we could disable the heuristic for C and keep things as-is for C++? Not entirely convinced of myself, but it's a start perhaps.

I can't speak to what the impact would be. I understand (or presume) that, short of an actual compiler, you're stuck playing a game of heuristics and tradeoffs, at least for C/C++, with its preprocessor complications. But that means that without some kind of configurability, you're essentially left to make an arbitrary choice of which users/codebases to support and which to break.

Would it be any more feasible to have a fixed (rather than configurable) list of excepted strings (e.g., UEFI, WINAPI, etc.)? Not perfect, but a crowdsourced, fixed list might be able to cover the 90% use case.

[rwols]

Could you shed any light on why it's not possible? E.g., is there no plumbing for parser preferences?

If I understand you correctly, no, we cannot change parser behavior through a user preferences file. However, the parser definition itself is just a YAML file. It's right here in this repository. You can also open the one that shipped with Build 3170 using the new "View Package File" command in the Command Palette. So it's "easily" editable. There's also this nifty plugin that could provide a solution.

Is the C/C++ parser not designed in a way that would be conducive to an exception list?

Would it be any more feasible to have a fixed (rather than configurable) list of excepted strings (e.g., UEFI, WINAPI, etc.)? Not perfect, but a crowdsourced, fixed list might be able to cover the 90% use case.

We can most definitely add an exception list in the YAML file somewhere, I'm okay with this approach.

But that means that without some kind of configurability, you're essentially left to make an arbitrary choice of which users/codebases to support and which to break.

Unfortunately yes :( it's going to be hit-and-miss with preprocessor macros forever.

[jsbmsu]

We can most definitely add an exception list in the YAML file somewhere, I'm okay with this approach.

Okay, then. I have no prior experience with .sublime-syntax files, but looking at the one for C, I see there is an existing variable for windows_types. That could form the exception list right now. We could easily add a uefi_types variable (or other special cases) and combine them to form an aggregate exception list.

The following simple change seems to work, though I cannot attest to whether it is the "right" way to do it:

  preprocessor-convention-ignore-uppercase-ident-lines:
    #- match: ^(\s*{{macro_identifier}})+\s*$
    - match: ^(\s*(?!{{windows_types}}){{macro_identifier}})+\s*$
      scope: meta.assumed-macro.c

[wbond]

My hunch is that we if we see a macro on a line by itself we need to push into a new context that allows identifier( to be a function definition. If it doesn't it pops out of that context and tries the normal processing.

I know it isn't pretty, but none of the C syntax is, we just need to do what we can to make it work in as many situations as possible.

[jsbmsu]

My hunch is that we if we see a macro on a line by itself we need to push into a new context that allows identifier( to be a function definition.

I lack the expertise to seriously evaluate the options or infer the complexity of what you're describing, but it certainly sounds more robust than what I have suggested.

Edit: Would what you're saying still allow the return-type-that-looks-like-a-macro to be matched (and colored) as a return type (or storage class, whatever you want to call it)? Or would it consume the initial identifier, scope it as assumed macro, and then find a function that has no return type?

[rwols]

My hunch is that we if we see a macro on a line by itself we need to push into a new context that allows identifier( to be a function definition. If it doesn't it pops out of that context and tries the normal processing.

I'm going to attempt and this and see how it goes.

[corthon]

@jsbmsu, I'm here for the UEFI types. :)

You never know how much you'll miss something until it's gone, and as a maintainer of MS Core UEFI (https://github.com/Microsoft/MS_UEFI) my week has been brought down dramatically by losing "goto symbol" in approximately ALL of my files.

Is there any light at the end of the tunnel for this?

[rwols]

@DeBTech I just made a PR with a candidate fix. I checked out the MS_UEFI repo and it seems to work:

[jsbmsu]

@rwols, thank you for working to address this so quickly!

Regarding the concern I raised:

Edit: Would what you're saying still allow the return-type-that-looks-like-a-macro to be matched (and colored) as a return type (or storage class, whatever you want to call it)? Or would it consume the initial identifier, scope it as assumed macro, and then find a function that has no return type?

Since neither EFI_STATUS nor EFIAPI (in the example screenshot you posted) are currently known to the syntax rules, could you please also try a test case with returning HRESULT (which is part of the windows_types variable) and confirm that it gets scoped/highlighted as a recognized type?

Or more generally, if I were to post an enhancement request (which I might implement myself, if I find time) to add common UEFI types (EFI_STATUS, UINT8, etc.) and UEFI modifiers (CONST, IN, OUT, EFIAPI, etc.), then in the UEFI example you posted, would EFI_STATUS and EFIAPI get detected and highlighted as a type and a modifier, respectively?

[corthon]

@rwols, thanks! I look forward to testing it. You've probably just saved me a lot of frustration.