hansec / fortran-language-server

Fortran Language Server for the Language Server Protocol
MIT License
295 stars 57 forks source link

preprocessor conditional code causing problems #78

Open emanspeaks opened 5 years ago

emanspeaks commented 5 years ago

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.

hansec commented 5 years ago

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.

emanspeaks commented 5 years ago

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?

hansec commented 5 years ago

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.

emanspeaks commented 5 years ago

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 commented 5 years ago

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

mscfd commented 5 years ago

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 commented 5 years ago

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.

hansec commented 5 years ago

@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 commented 5 years ago

@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.

mscfd commented 5 years ago

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_forxyz -> real(8)), which should be preserved so that reported type signatures are still _real_forxyz and not real(8)).

Thanks for the effort!

hansec commented 5 years ago

@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 commented 5 years ago

@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.

emanspeaks commented 5 years ago

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?

hansec commented 5 years ago

@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.

mscfd commented 5 years ago

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.

hansec commented 5 years ago

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

mscfd commented 5 years ago

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.

hansec commented 5 years ago

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?

mscfd commented 5 years ago

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?