erl_syntax_lib: annotate maybe_match_expr and else_expr correctly

[MarkoMin]

maybe_match_expr and else_expr were not handled properly when annotating bindings. This was causing problem with ELS and this is currently preventing me to fix the issue. The problems were both false positive and false negative diagnostics.

This will probably cause problem in ELP too, so I'll kindly ping @robertoaloi @michalmuskala to take a look at this.

I didn't provide any test for this because it seems that erl_syntax_lib is not tested.

What one can do to test this PR is to build Erlang-LS from this PR and make sure that els_diagnostics_SUITE:bound_var_in_pattern_maybe/1 testcase passes. Testcase tests for both false negatives and false positives.

What I'm not completely sure is whether:

f() ->
maybe
    X ?= foo
else
    Err -> Err
end,
X.

should return bound variable or unsafe variable diagnostic? Currently it returns bound variable, but it probably should be unsafe. I'll add that to some testcase once I know what the desired behavior is.

[github-actions]

CT Test Results

  2 files   13 suites   4m 41s ⏱️
113 tests 111 ✅ 2 💤 0 ❌
129 runs  127 ✅ 2 💤 0 ❌

Results for commit 95b849d.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[MarkoMin]

Currently it returns bound variable, but it probably should be unsafe. I'll add that to some testcase once I know what the desired behavior is.

I just re-check and the code already results in "unsafe in maybe" error - which I think is correct behavior.

[bjorng]

Thanks for your pull request.

Since this is a bug fix, I think it is appropriate to base it on the maint branch. If based on master, this fix will be released in Erlang/OTP 28, which is planned to be released next spring.

Could you also add a test to the syntax_tools_SUITE? We usually add a test when a bug is fixed. As an example of writing a test case for testing annotations, there is test_named_fun_bind_ann/1, which was added by you in 2022.

While you are it, please squash the commits into one commit. It would also be nice if you can put some of the information from the description of this PR into the commit message.

[bjorng]

Your new test case will not actually be run unless you add its name to the end of the list of test cases in all/0. When you have done this change, please squash the commits down into one commit.

[MarkoMin]

Finally found some time for this. I think now it's correct. If I understood it correctly, no variables are bound after maybe.

I'm not completely sure whether Error variable in:

else
   Error -> Error

should be free or not? Currently is neither free nor bound. Is this correct behavior?

FWIW: I've tested this quickly over few examples with erlang_ls that previously caused errors and now it works as it should.

[bjorng]

Thanks for your pull request.