fortran-lang / fortls

fortls - Fortran Language Server
https://fortls.fortran-lang.org/
MIT License
258 stars 41 forks source link

Support selected Intel-specific preprocessor macro expansions #341

Open emanspeaks opened 11 months ago

emanspeaks commented 11 months ago

~Closes issue 297~ ~Addresses spaces on either side of the hash in preprocessor lines~ (Edit: this PR has been repurposed after refactoring. The previously-cited description now applies to #350.)

Addresses Intel compiler extensions present due to legacy support for older compiler versions. These are particularly prolific in their implementation of Fortran standard library modules and system/platform-specific modules, but for which it would be desired to have hover capabilities for those users working in the Intel ecosystem.

codecov[bot] commented 10 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (71d5f40) 87.51% compared to head (c975006) 87.79%.

Files Patch % Lines
fortls/langserver.py 88.88% 1 Missing :warning:
fortls/parsers/internal/parser.py 98.73% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #341 +/- ## ========================================== + Coverage 87.51% 87.79% +0.27% ========================================== Files 35 35 Lines 4756 4824 +68 ========================================== + Hits 4162 4235 +73 + Misses 594 589 -5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

emanspeaks commented 10 months ago

I'm not sure what to do about the warning on the regex above, but otherwise this seems to be working like a champ. I look forward to your comments and/or your merging hopefully soon :-)

gnikit commented 10 months ago

I see a lot of changes that are seemingly unrelated in this PR. My expectation would be that you would only need to add [ ]* to some regex expressions? Can you elaborate as to the other modifications?

emanspeaks commented 10 months ago

I buried the lede perhaps, but did rename the PR to say it is performing full macro expansions for function-like macros. That was actually the intent of this PR and the whitespace changes were just a bonus. I will change the name again to be even more explicit about what is going on here. This resulted in a need for updated/new unit tests, which are the remaining updates.

In order to also support lines containing tabs in addition to spaces, I chose to use \s* instead of [ ]*. Those changes should be obvious in regex_patterns.py.

emanspeaks commented 10 months ago

Sorry, should have included this in my last message, but let me elaborate on specifically how I do this. The updated PP_DEF regex now captures parenthesis if they exist next to a macro name. These are captured in regex group 3 (inclusive of parentheses). Group 4 is the contents of the parens (i.e. the arguments of the macro function).

If regex group 3 contains a match, parens are present in the macro name, and rather than just adding the string of the #define's "value" to the pp_defs dict, it adds a tuple of the arguments (group 4) and THEN the rest of the line.

In later code that parses out values from pp_defs dicts, I add code to detect if the contents are a tuple rather than simple string. If so, then the expansion is performed as necessary. Otherwise, it behaves just as before.

I searched the code for everywhere it seemed like pp_defs or something similar were being referenced, so all the bases should be covered on handling the tuple where previously it was only a string.

I added unit tests for these expansions as well as testing for additional whitespace in PP directives, which themselves broke other unit tests, and I made a number of changes as a result of expected changes due to the new unit test and code changes. The only test that continued to fail was one regarding server update, which was the result of my running the tests in a conda environment. It is expected that the update will not be performed when tests are run in a conda environment, so that test checks if the test environment is a conda environment and changes the expected test value if so. Another Easter egg update I forgot about until reviewing the changes again just now, ha ha.

gnikit commented 10 months ago

In order to also support lines containing tabs in addition to spaces, I chose to use \s* instead of [ ]*. Those changes should be obvious in regex_patterns.py.

\s contains a lot more that just ` and\t` characters, see and that is a problem. The rest of the parser does not support tabs, so allowing for tabs in a small section of the parser is a separate issue and a separate PR. For now whitespaces should be used.

I will have a detailed look at the PR but I am not sure the proposed solution works for all cases

emanspeaks commented 10 months ago

I can replace the \s with spaces, happy to do that.

At a minimum, this implementation should be consistent with Intel and gfortran conventions. I can't speak to any other preprocessors. I am happy to continue improvement to support others if there are cases that this doesn't work, I just need test cases for those and I can add to the unit tests. Now that this infrastructure is in place for doing the expansions, it shouldn't difficult to tweak as needed for other edge cases.

emanspeaks commented 10 months ago

I made the requested update above to only check for spaces. I have subsequently come across some code that depended on Intel's compiler directives, so I added an option here to support that (defaults to disabled, Intel users would need to opt in).

gnikit commented 10 months ago

I haven't forgotten about this @emanspeaks, I have a mostly-finished review (that is now outdated since the last push). It is in my TODO list and I will try and finish today, but I am quite busy (so no promises) I will make it :worried:

gnikit commented 10 months ago

I've noticed the PR now includes Intel-specific options. Let's break down these features into separate PRs to keep things manageable and focused.

Intel Options

Remember, our language server is vendor-neutral. We can't start adding compiler-specific features; it's not sustainable. If Intel directives fit with our current preprocessor, great, let's include them. If not, we can't realistically rewrite the preprocessor for both cpp and fpp. So, these changes might not be feasible for fortls.

We should discuss this further, but as a separate issue and PR.

For this PR, what are the features that are vendor agnostic and are responsible for the macro expansion? These should be the focus. Let's cherry-pick and either update this PR or open a new one with only these features.

emanspeaks commented 10 months ago

Yeah, this morphed rather organically from where this branch started. I agree it makes sense to split this up. It shouldn't be terribly difficult to take care of, and will try to do that this weekend.

emanspeaks commented 10 months ago

Given that the name of this branch I'm working from here is named ifort-fpp-fixes, it made more sense to me to use that branch for the vendor-specific changes for Intel. GitHub won't let you change the branch associated with a PR, but I didn't want to close this one either. If you'd rather I do that and we get a clean start with this branch in a new PR, I can do that instead.

Assuming, though, it's ok to proceed here, let me reconcile this branch now with the forked changes in #350 and will let you know when this is ready for review again.

emanspeaks commented 10 months ago

Ok, I think the changes are complete. Since this branch contains both its own contents and #350 's changes, looking at the changed files here isn't the most helpful until/unless you merge that one first. If you want to see the Intel-specific changes relative to #350, you can look here