Don't show completions on nested module identifier

[kerams]

A simple fix.

moduleNamespace.TopmoduleNest // =

This test is failing because without the equals sign Nest does not appear in the syntax tree at all. What exactly would it take to make this happen? I'm assuming something in the parser. Unfortunately, I am not yet familiar with that part of the compiler.

[dsyme]

Code looks ok but the new test is still failing

test still failing

[kerams]

I know and I have described why that is. Any clue on how to go about what I'm asking for?

[kerams]

Would it be necessary to add isFromErrorRecovery to SynModuleDecl.NestedModule (a la SynExpr.IfThenElse) to that end? Adding FromParseError does not seem like a good fit here.

For the sole reason that this nested module would appear in the syntax tree

namespaceNmoduleNested

[kerams]

Bump?

[psfinaki]

/azp run

[vzarytovskii]

/run fantomas

[github-actions]

Ran fantomas: https://github.com/dotnet/fsharp/actions/runs/3361542341

[psfinaki]

@kerams if you want I might help with that later this week.

[kerams]

Well, first I want to know whether we're OK with extending the syntax tree just for this use case. If that isn't the case, the PR can be closed. Otherwise I only need a tip on the preferred approach with regards to modelling the error case.

[psfinaki]

Well from what I understand @dsyme is fine with that and nobody is against in general :)

[kerams]

I've added recovery for incomplete nested modules.

Before:

After:

@auduchinok, @nojaf , before I add tests and extend this to signature files, can you please check if the changes look sound to you?

[kerams]

Ugh, it doesn't work with ML compatibility mode

#indent "off"moduleMletSimpleSample()=1

Is there a way to simulate a pattern guard in yacc?

| opt_attributes opt_access moduleIntro // when not mlSupport

Or maybe it's time this long-deprecated syntax was finally removed in .NET 8 SDK? :)

[nojaf]

@auduchinok, @nojaf, before I add tests and extend this to signature files, can you please check if the changes look sound to you?

I think this looks fine, though I'm less familiar with recovery. I'll let @auduchinok take this one.

[kerams]

If I add recover at the end, #indent "off" works, but the original error on the following declaration is preserved. It's not a big deal, but it does not add any value with the new diagnostic.

[auduchinok]

If I add recover at the end, #indent "off" works, but the original error on the following declaration is preserved. It's not a big deal, but it does not add any value with the new diagnostic.

If all is working by itself with recover added, then the additional error shouldn't be needed. Are there any other issues with recover added?

[kerams]

The Moon and the stars have aligned, ready for review.

[nojaf]

As you changed the pars.fsy grammar rules, I would very much like to see a test in https://github.com/dotnet/fsharp/blob/main/tests/service/SyntaxTreeTests.fs where this new AST is constructed. Could you please an example file and a baseline file to https://github.com/dotnet/fsharp/tree/main/tests/service/data/SyntaxTree?

[nojaf]

Thanks @kerams 🙏

[auduchinok]

@kerams I wonder if changes to LexFilter are still needed after you've changed recovery to use the builtin logic?

[T-Gro]

(updating to get the "short ranges" string representation in)

[psfinaki]

@kerams this is ready to merge, right?

[kerams]

Yup.

[psfinaki]

Cool, thanks a lot!