suggest: refactoring outline to parser based

[bung87]

Summary

• Remove outline processing from procedure suggestSym, change to
  parser based processing
• Outline is meant to outline the document, not compiled AST.

Details

• Export proc outline*(graph: ModuleGraph; fileIdx: FileIndex) processing
  full document.
• Add flags*: set[SuggestFlag] field to Suggest, allow mark symbol as
  deprecated.
• Disable nimsuggest/tests/tinclude.nim which used for old implementation
  that require compiled included file.
• Add nimsuggest/tests/toutline.nim for testing new implementation.

---

Notes for Reviewers

• part of tool: LSP support #879

[zerbina]

Your PR messages are very much improving, good work!

Two comments that I have without having looked at the changes yet:

• the "tool: suggest x" title reads strange, as it sounds like something is suggested. The "suggest:" prefix is less ambiguous
• since this is a breaking change, please document the impact. Both the user-facing impact (that is, how are users of nimsuggest affected?) and the impact on the documentation need to be described

[bung87]

the outline used in nimsuggest.executeNoHooks which is not exported procedure, I think it's fine.

[zerbina]

I did a first pass over the changes, with the largest focus being on code style.

[zerbina]

I think it'd be better if the logic above is moved into a separate procedure (e.g., markDirtyFile), with executeCmd only being called if cmd is not ideOutline.

[zerbina]

the outline used in nimsuggest.executeNoHooks which is not exported procedure, I think it's fine.

It needs to be clear what changes. You disabled the tinclude.nim test, which is a direct indicator of user-facing behaviour changing significantly.

There's also missing a reason/motivation as to why the outline command is changed. That is, elaboration is missing on why outline should work on the document and not on the AST.

---

Regarding the deprecated detection and flag: I think it's not a good idea. Whether something is deprecated or not is, in my opinion, a property of a symbol, but no symbols exist at the stage where outlining now happens. The deprecated pragma can also be removed from a definition through macros.

[zerbina]

Please also make sure that the code is documented. At least the routines, types, and constants should have a documentation comment attached.

[bung87]

the outline used in nimsuggest.executeNoHooks which is not exported procedure, I think it's fine.

It needs to be clear what changes. You disabled the tinclude.nim test, which is a direct indicator of user-facing behaviour changing significantly.

There's also missing a reason/motivation as to why the outline command is changed. That is, elaboration is missing on why outline should work on the document and not on the AST.

Regarding the deprecated detection and flag: I think it's not a good idea. Whether something is deprecated or not is, in my opinion, a property of a symbol, but no symbols exist at the stage where outlining now happens. The deprecated pragma can also be removed from a definition through macros.

• it's a weird idea outline the included file or expanded ast while users just want navigate between document sections
  also parser based allow get the result earlier.
• there is a concept of DocumentSymbols (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#documentSymbol), which contain a tags field. it has Deprecated field

see the current LSP server screenshot, the vscode style the deprecated symbol through line

[zerbina]

I was rather tired yesterday, and thus my comments and review were only focused on surface-level details, apologies.

---

The direction this PR takes is, in my opinion, wrong. I agree that outlining should only report the symbols as they appear in the document (so no symbol coming from includes, macros, generic instantiation, etc.), but using a parser-based solution is not how this should (and can) be achieved.

After parsing, you only have identifiers, and trying to guess/approximate what symbols will result from them is cannot work reliably, especially with the amount of meta-programming on definitions that NimSkull allows for.

Phrased differently: I think that the AST-based processing needs to stay. In suggestSym, not retrieving the parent module for symbols declared within included code should be enough to achieve included symbols not being reported with the outline command.

• it's a weird idea outline the included file or expanded ast while users just want navigate between document sections
  also parser based allow get the result earlier.

I agree with the first half of your statement, but having result be produced faster is of little use when the results are incorrect.

• there is a concept of DocumentSymbols (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#documentSymbol), which contain a tags field. it has Deprecated field

I strongly agree that outlining should report whether a symbol is marked as deprecated. However, as I said, it's only possible to know which entities are deprecated once you have access to symbols (through looking for the sfDeprecated flag). The name DocumentSymbols also hints at this.

Approximating this by looking for deprecated identifiers appearing within pnkPragmaExpr nodes is not a solution.

For example:

template drop() =
  discard

var def {.deprecated, drop.} = 1

Here, def is not something that should be produced by outlining.

[bung87]

fair enough, even the example code rarely seen. don't know by what use case the user want to rewrite the pragms

[zerbina]

fair enough, even the example code rarely seen. don't know by what use case the user want to rewrite the pragms

In the end, it's only relevant whether something is allowed by the language. Also, while changing pragmas post-declaration might be rarely used (if ever), dropping a definition by way of a template/macro is a realistic use case.

In addition, to name a few more cases, a parser-based solution cannot support:

• when statements
• {.push deprecated.}
• {.pragma myDeprecated: deprecated.}