fortran-language-server icon indicating copy to clipboard operation
fortran-language-server copied to clipboard

preprocessor conditional code causing problems

Open emanspeaks opened this issue 6 years ago • 19 comments

The following is a valid code excerpt from a project:

#ifndef USE_PYQT
        select case(message%value1)
#else
        select case(message)
#endif

However, it seems fortls is simply ignoring the preprocessor directives and treating the second select case as a nested select, which is causing parsing problems/lot of unexpected end of scope diagnostic errors.

emanspeaks avatar Feb 06 '19 06:02 emanspeaks

Unfortunately, this is a tricky situation since it relies on more complete preprocessor handling in the language server. I plan to add more preprocessor functionality in the future, but I haven't had the time yet. In the near term I think I will end up just trying to identify the "default" preprocessor paths if nothing is defined and only parse those regions, which I believe will solve your issue.

hansec avatar Feb 06 '19 15:02 hansec

I think that's a reasonable solution, but I'd like the ability to specify the preprocessor options in the .fortls file or something to force it to parse one or another.

Does fortls currently evaluate the directives themselves if, say, they define a macro used somewhere in the code?

emanspeaks avatar Feb 06 '19 22:02 emanspeaks

Ok, I just pushed a simple implementation of this to a testing branch (simple_preproc). It supports defining preprocessor variables in the .fortls file and tracks #define and #undef statements in files. Note that it only supports conditionals that check if a single variable is defined or not. Take a look and see if this works for your use case.

hansec avatar Feb 07 '19 18:02 hansec

This looks like it is working for me for now, thanks so much! As mentioned above, we do have some preprocessor macros and other conditionals that would be good to add as an enhancement later, but at least for now the rootpath for my project is finally free of all errors!

emanspeaks avatar Feb 07 '19 18:02 emanspeaks

In your readme.rst example using pp_defs, you need a comma at the end of that line :-)

emanspeaks avatar Feb 07 '19 18:02 emanspeaks

The following small example code (which uses a macro to turn on/off pure) crashs the parser (version 1.1.1) with a ValueError. First the parser should not crash, as this could also be triggered by some syntax error. But more importantly it would be nice to have support for such macros (in this case, just ignore the macro).

module xxx

implicit none private

#define PURE pure

contains

PURE function fun1() result(x) integer :: x x = 0 end function fun1

PURE function fun2() result(x) integer :: x x = 0 end function fun2

end module xxx

mscfd avatar Feb 13 '19 08:02 mscfd

Arghh, the PURE macro is printed italic instead with underlines. 'insert code' does not seem to help here. Anyway PURE in the code above should be underlinePUREunderline.

mscfd avatar Feb 13 '19 08:02 mscfd

@mscfd Thanks for the report. However, I am unable to replicate the parser crash you see for the given code (repeated below with hopefully correct formatting). The parser reports errors in the file due to unclosed scopes, but does not crash.

I will work on adding support for evaluating defined preprocessor variables in the parser. However, depending on how complicated the definition flow is in your real code full support might be a while off. Something simple like this will be relatively easy, but if the definition is being #include'd or coming from a USE'd file that is a lot trickier.

module xxx

implicit none
private

#define _PURE_ pure

contains

_PURE_ function fun1() result(x)
integer :: x
x = 0
end function fun1

_PURE_ function fun2() result(x)
integer :: x
x = 0
end function fun2

end module xxx

hansec avatar Feb 13 '19 16:02 hansec

@mscfd and @emanspeaks I just pushed an improved version of preprocessing to the branch cpp_if_parsing. Check it out and see if that improves things for you or causes more problems. Thanks again for the input/suggestions.

hansec avatar Feb 13 '19 17:02 hansec

I must have changed something I cannot reproduce the ValueError with just two functions anymore. But with three I get the exception. So just add another _PURE_ function fun3() ... by copy and paste and you should see the problem. Sorry for the wrong reproducer. BTW, I have checked it with "fortls --debug_rootpath . --debug_parser --debug_filepath xxx.F90", where xxx.F90 is the test fortran module. With just two function, no parser error is reported, but I can see that it does not know about fun1 and fun2.

Your commits do not resolve the problem. I made sure that the PURE macro is read from .fortls and is in pp_defs dictionary where it is copied to defs_tmp for parsing in parse_fortran.py (line 594). Anyway, I have simply hardcoded the problematic macros in some regexes and then it seems to work (on a big code basis), so I can use it. (We have some macro which look like (real_for_xyz -> real(8)), which should be preserved so that reported type signatures are still real_for_xyz and not real(8)).

Thanks for the effort!

mscfd avatar Feb 14 '19 05:02 mscfd

@mscfd Thanks, I am able to reproduce the error now. I will try and correct this type of issue for general syntax errors as you mentioned.

I think I found the issue with the __PURE__ macro example above in the new branch. Macro replacement wasn't working when the macro occurred at the start of a line. Can you check out the new version? As for showing the macro and not its replacement in other language results, that will be a bit trickier. If you have control over the source code I would suggest using real(KIND=real_for_xyz) instead of replacing the whole declaration. Although, obviously this won't work if the type changes too, say real(8) to complex(8).

hansec avatar Feb 14 '19 13:02 hansec

@mscfd I think I was able to fix the parser crash issue you mentioned. Although, of course a syntax error like this leads to lots of diagnostics errors. Hopefully, they are helpful though.

I also released (v1.2.0) that fix and the preprocessor improvements. As mentioned above, I believe I was able to solve the remaining macro issue you mentioned. That's what I get for not testing on your test case before I pushed the initial changes. Thanks again for the input and let me know if you run into any more issues.

hansec avatar Feb 14 '19 17:02 hansec

Did you still want me to try using the cpp branch or just upgrade to 1.2.0 and tell you if I find anything there?

emanspeaks avatar Feb 14 '19 18:02 emanspeaks

@emanspeaks Go ahead and upgrade to 1.2.0, it is the most up to date. Also, note that I changed the way preprocessor variables are defined in the .fortls file to a dictionary to allow setting values not just defined/undefined. See the README for more info.

hansec avatar Feb 14 '19 18:02 hansec

Just checked. The 1.2.0 tag works and replaces the macros properly. For example, we have a "DIM1" macro for "dimension(:), contiguous", which also works. For a few macros I will still use some minor patches, as the macro are used as type-synonyms, a feature unfortunately lacking in fortran.

mscfd avatar Feb 21 '19 11:02 mscfd

@mscfd Great, if you have an example of the "type-synonym" macro I'd be happy to take a look at that as well.

hansec avatar Feb 21 '19 15:02 hansec

As mentioned above: Take for example _real_stencil_ or maybe even real(kind=_kind_stencil_), as a synonym for real(4) or real(8). It allows to configure stencil precision at compile time, but it also serves to document that a declared value is a stencil value. Another example _VEC3_ for real(8), dimension(1:3) as a very common code pattern.

mscfd avatar Feb 22 '19 12:02 mscfd

Ok, just to be clear these macros don't cause the language server to crash right? Are your patches to show the code "as written" (including pre-substitution macros) in autocomplete and other language server results?

hansec avatar Feb 22 '19 17:02 hansec

Emacs shows the _real_stencil_ type just as expected. It should be, as it has no knowledge of the language itself, it just echos what it is served by the language server, right?

mscfd avatar Feb 26 '19 09:02 mscfd