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

Ada: fixing highlighting for Ada aspects #2125

Merged
merged 3 commits into from
Jun 16, 2022

Conversation

gusthoff
Copy link
Contributor

Aspects were introduced in Ada 2012.

@gusthoff gusthoff marked this pull request as draft April 29, 2022 15:29
@gusthoff gusthoff force-pushed the topic/ada/aspects_fix branch from 9ad800b to bec1f56 Compare April 29, 2022 15:45
@gusthoff gusthoff marked this pull request as ready for review April 29, 2022 15:56
@jeanas
Copy link
Contributor

jeanas commented Apr 29, 2022

Could you elaborate on the rationale for this change? Even if Name.Namespace is not fully correct, it may have been chosen in order to allow styles to highlight it in a different color than other names. This sometimes happens because it's impossible to design a set of token types that covers all relevant concepts in such a variety of languages as that of those Pygments supports.

@gusthoff
Copy link
Contributor Author

gusthoff commented Apr 29, 2022

I'm not sure how useful Name.Namespace is in the case of Ada. (I personally don't see a big advantage in highlighting the name in cases such as with System;.)

In any case, the current parsing is simply wrong when it comes to Ada aspects. Take this code, for example:

   procedure Initialize (Size : Integer)
     with
       Import        => True,
       Convention    => C,
       External_Name => "registerInterface_Initialize";

Here, the current version of the parser highlights the first name (Import) and doesn't highlight the remaining aspects. With the change I'm proposing, the mismatch disappears.

@jeanas
Copy link
Contributor

jeanas commented May 6, 2022

Sorry for the delay. I'm still not understanding this change well: the use keyword takes you in a state called 'import', but that state is not necessarily an import? Wouldn't at least a renaming be warranted? What is the 'import' state useful for now?

Is it possible to recognize the syntax better? Just looking at your example, it looks like a lookahead assertion [\w.]+(?!\s*=>) could do the trick. Is the syntax richer than that?

@gusthoff
Copy link
Contributor Author

gusthoff commented May 6, 2022

Another example of Ada aspects could be this:

   procedure Update (X : in out Integer)
     with Inline;

In this case, the lookahead you're suggesting wouldn't work.

@jeanas
Copy link
Contributor

jeanas commented May 9, 2022

OK. Then what is the 'import' state for now?

@gusthoff
Copy link
Contributor Author

I'm not sure. I guess import can still be useful if full support for parsing Ada aspects gets implemented (which is not what this PR is doing here: it's just fixing a nasty highlighting issue in the least intrusive way possible).

@gusthoff
Copy link
Contributor Author

gusthoff commented Jun 3, 2022

Could you please let me know what's the next step from here?

@jeanas
Copy link
Contributor

jeanas commented Jun 3, 2022

This seems to be removing a feature in order to fix a bug related to the feature. I'm honestly not sure this is a good approach. It can be if the feature has no chance of being fixed.

Does it take a lot of work to fix this fully while retaining the special Name.Namespace special highlighting? If so, I'd rather see the 'import' state removed and a comment that parsing that is too tricky and thus not done.

@gusthoff
Copy link
Contributor Author

Actually, before submitting this pull request, I already tried to come up with a solution that would include full support for Ada aspects. However, it seemed much more complex than anticipated.

Today, I tried to find a way to minimize the impact of my PR, but I haven't succeeded in that. To be honest, I doubt there's an easy way to solve this issue without a major refactoring of the Ada lexer.

@jeanas
Copy link
Contributor

jeanas commented Jun 16, 2022

SGTM now, but fails testing. Should test_ada2022.adb.output be test.adb.output?

@gusthoff gusthoff force-pushed the topic/ada/aspects_fix branch from cb6b790 to f3bd68c Compare June 16, 2022 14:57
@gusthoff
Copy link
Contributor Author

Sorry, I just tried out removing the import state, but it just makes it worse. (When removing it, names such as Ada.Text_IO aren't recognized anymore; instead, it's tokenized to Name, Punctuation, Name, which is confusing IMHO.)

@jeanas jeanas merged commit 2bcec67 into pygments:master Jun 16, 2022
@jeanas
Copy link
Contributor

jeanas commented Jun 16, 2022

I'll merge this and add a comment.

@gusthoff
Copy link
Contributor Author

Thanks!

@Anteru Anteru added this to the 2.13.0 milestone Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants