Skip to content

Add resyntax integration #153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

6cdh
Copy link
Contributor

@6cdh 6cdh commented Jul 8, 2025

This is a initial implementation for #40 .

Some screenshot:

Screenshot 2025-07-08 232519

Screenshot 2025-07-08 232529

Screenshot 2025-07-08 232544

Todo:

  • Improve quick fix description
  • Implement error handling for unsupported languages
  • Incompatibility for old Racket version
  • Add a setting to allow editor to enable or disable
  • Add test
  • Improve latency

For code, I added a new function walk-text which provides updated text string to services, it is called every time check-syntax succeeds.

@6cdh
Copy link
Contributor Author

6cdh commented Jul 8, 2025

And what do you think user defined rules? Should we support it? And how to support it?

@dannypsnl
Copy link
Contributor

I have a similar question, if user didn’t like a certain suggestions, where can they disable it?

It seems we also need magic-racket they give us those configurations?

@dannypsnl
Copy link
Contributor

dannypsnl commented Jul 8, 2025

misc

  • test rhombus (compatible, will not raise error)
  • update CI

@jackfirth
Copy link
Contributor

For user-defined rules, I don't think supporting that should be the editor's responsibility. That should be integrated into Resyntax proper - possibly through some mechanism where some info.rkt config tells Resyntax where to find the rules.

@jackfirth
Copy link
Contributor

@dannypsnl Resyntax's goal is to not provide suggestions that people frequently want to disable. So far, that's mostly been working. If there are suggestions that people don't find to be helpful, I would rather they file issues in the Resyntax github repository than disable them themselves. But see also some of my thoughts for suppressing suggestions within Resyntax itself: jackfirth/resyntax#436.

@dannypsnl
Copy link
Contributor

@jackfirth I think the difference is that, code actions are annoying in editor, so they better show up only when no one will argue that's a style problem.

For another part. Indeed, if resyntax can search user config than no need to change magic-racket, configure options in vscode is painful.

@jryans
Copy link
Contributor

jryans commented Jul 9, 2025

First off, this a neat feature, thanks for working on it! 😄

I do think some means to at least to disable suggestions entirely is needed, as not everyone will want them to appear.

The main pathways I can imagine are:

  • editor setting which is passed along to the lang server
  • some resyntax-layer config in a file somewhere

I can also imagine some people wanting more fine-grained control beyond just off / on, but that's perhaps a future refinement.

Looking at other language ecosystems, Rust Analyzer takes the first path of having quite a few editor-level settings which are then passed to the lang server. IIRC, we don't have any concept of that for Racket so far, but it seems like a reasonable way for users to communicate their preferences for tooling features like this.

@jackfirth
Copy link
Contributor

Just on or off would be the perfect setting to have in the langserver / editor / what-have-you. Fine-grained configuration should definitely be handled at the Resyntax layer without editors or langservers needing to do anything.

@6cdh
Copy link
Contributor Author

6cdh commented Jul 20, 2025

Racket-langserver now requires minimum version of Racket 8.16, from the previous 7.6. Because resyntax uses treelist (8.13) and sequence->treelist (8.16).

I don't know if this is good and is there any way to avoid this requirement?

@6cdh
Copy link
Contributor Author

6cdh commented Jul 20, 2025

For unsupported language files, resyntax can skip them automatically, so we don't need to take any action.

But every time resyntax skips a file, it always print this log to stderr

resyntax: skipping string source
 encountered an error during macro expansion
  error:
   ...

This log cannot be disabled. It could be a problem when debug.

@6cdh
Copy link
Contributor Author

6cdh commented Jul 20, 2025

I added settings to enable editor control features. By default it's this:

{
    "resyntax": {
        "enable": true
    }
}

@jryans Please test if it works if you have time.

@dannypsnl
Copy link
Contributor

@6cdh You can use dynamic-require to detect if required stuff is there or not, for example

((with-handlers
     ([exn? (λ (e) (λ (_) #f))])
   (dynamic-require (list 'lib "racket/treelist") 'sequence->treelist1))
 (list 1 2 3))

If sequence->treelist is not there, got #f, otherwise got (treelist 1 2 3)

@dannypsnl
Copy link
Contributor

For unsupported language files, resyntax can skip them automatically, so we don't need to take any action.

If we create another function, e.g. resyntax-analyze-lsp which use different skip function?

@jryans
Copy link
Contributor

jryans commented Jul 20, 2025

I added settings to enable editor control features. By default it's this:

{
    "resyntax": {
        "enable": true
    }
}

@jryans Please test if it works if you have time.

Thanks, I will endeavour to take a look, but it may take me a while (recent illness plus too many things to do), so please don’t feel you need to wait on me.

@jackfirth
Copy link
Contributor

Resyntax has so far been built on the assumption it's running in a CI environment where it's easy to use the most recent Racket version. You won't be able to get Resyntax working on an older Racket version from the outside. It would need some fairly extensive work done on Resyntax itself.

@6cdh
Copy link
Contributor Author

6cdh commented Jul 21, 2025

@dannypsnl

You can use dynamic-require to detect if required stuff is there or not

Resyntax probably also uses other new features, and only detect treelist is not enough. But maybe we can use dynamic-require to load Resyntax dynamically. If Resyntax exists and load correctly, enable it, otherwise disable it. I don't know if it's possible, probably need some experiment.

If we create another function, e.g. resyntax-analyze-lsp which use different skip function?

I don't understand your meaning. In this code

(define result-set
  (resyntax-analyze
    text-source
    #:suite default-recommendations
    #:lines all-lines))

If the text-source is unsupported language, resyntax-analyze skips the source and returns a empty result set. So we don't need to handle the unsupported case explicitly.

@dannypsnl
Copy link
Contributor

@6cdh Indeed, dynamic require resyntax is enough.

I mean the stderr output you mentioned, that’s from an resyntax internal logging function.

But every time resyntax skips a file, it always print this log to stderr

It checks if resyntax is available in the environment and uses it if so.
This commit also restore CI and info.rkt to keep old Racket
compatibility.
@6cdh
Copy link
Contributor Author

6cdh commented Jul 25, 2025

If we create another function, e.g. resyntax-analyze-lsp which use different skip function?

Oh you mean this skip function

Yes, I agree.

@jackfirth
Copy link
Contributor

If we create another function, e.g. resyntax-analyze-lsp which use different skip function?

Oh you mean this skip function

Yes, I agree.

That's using a normal racket logger with the topic 'resyntax, so if you want to suppress that output you can do so through racket's usual logger config mechanisms, like the PLTSTDERR environment variable.

@6cdh
Copy link
Contributor Author

6cdh commented Jul 27, 2025

That's using a normal racket logger with the topic 'resyntax, so if you want to suppress that output you can do so through racket's usual logger config mechanisms, like the PLTSTDERR environment variable.

I know. But these stderr messages are sent to the editor. If users enable lsp tracing, and see theses messages, they probably think this is an error that needs attention. But it's actually not. Because it's common to open an unsupported file with lsp (for example, typed racket), and resyntax is expected to not work in this case.

I prefer it does not show logs by default and only shows by given this environment variable or some explicit signals.

This is probably a difference when it being a command line tool between an internal component.

@jackfirth
Copy link
Contributor

I prefer it does not show logs by default and only shows by given this environment variable or some explicit signals.

You can change the value of PLTSTDERR to do that. It defaults to error, but if the racket langserver sets it to error fatal@resyntax before starting the racket process, that should silence it.

@6cdh
Copy link
Contributor Author

6cdh commented Jul 27, 2025

You can change the value of PLTSTDERR to do that. It defaults to error, but if the racket langserver sets it to error fatal@resyntax before starting the racket process, that should silence it.

Doesn't it require all clients to change the way they starts racket-langserver? (env PLTSTDERR="error fatal@resyntax" racket -l racket-langserver

@jackfirth
Copy link
Contributor

Hmm. Yes, that's an issue.

Thinking about this more, the problem is that it logs these errors when analyzing files in unsupported languages, correct? I think I can just fix resyntax to handle that more gracefully in general, without logging an error.

@6cdh
Copy link
Contributor Author

6cdh commented Jul 27, 2025

the problem is that it logs these errors when analyzing files in unsupported languages, correct?

Yes, that's the problem.

jackfirth added a commit to jackfirth/resyntax that referenced this pull request Jul 27, 2025
When `resyntax-analyze` is called directly on a non-`#lang racket` file, it currently results in an error during macro expansion being raised and logged. This causes issues when the langserver tries to analyze the current file with Resyntax, see jeapostrophe/racket-langserver#153. This PR moves Resyntax's logic for handling such files out of the file group resolution code and into the core of `resyntax-analyze`.
jackfirth added a commit to jackfirth/resyntax that referenced this pull request Jul 27, 2025
When `resyntax-analyze` is called directly on a non-`#lang racket` file, it currently results in an error during macro expansion being raised and logged. This causes issues when the langserver tries to analyze the current file with Resyntax, see jeapostrophe/racket-langserver#153. This PR moves Resyntax's logic for handling such files out of the file group resolution code and into the core of `resyntax-analyze`.
jackfirth added a commit to jackfirth/resyntax that referenced this pull request Jul 27, 2025
When `resyntax-analyze` is called directly on a non-`#lang racket` file, it currently results in an error during macro expansion being raised and logged. This causes issues when the langserver tries to analyze the current file with Resyntax, see jeapostrophe/racket-langserver#153. This PR moves Resyntax's logic for handling such files out of the file group resolution code and into the core of `resyntax-analyze`.
jackfirth added a commit to jackfirth/resyntax that referenced this pull request Jul 27, 2025
When `resyntax-analyze` is called directly on a non-`#lang racket` file, it currently results in an error during macro expansion being raised and logged. This causes issues when the langserver tries to analyze the current file with Resyntax, see jeapostrophe/racket-langserver#153. This PR moves Resyntax's logic for handling such files out of the file group resolution code and into the core of `resyntax-analyze`.
jackfirth added a commit to jackfirth/resyntax that referenced this pull request Jul 27, 2025
When `resyntax-analyze` is called directly on a non-`#lang racket` file, it currently results in an error during macro expansion being raised and logged. This causes issues when the langserver tries to analyze the current file with Resyntax, see jeapostrophe/racket-langserver#153. This PR moves Resyntax's logic for handling such files out of the file group resolution code and into the core of `resyntax-analyze`.
jackfirth added a commit to jackfirth/resyntax that referenced this pull request Jul 27, 2025
When `resyntax-analyze` is called directly on a non-`#lang racket` file, it currently results in an error during macro expansion being raised and logged. This causes issues when the langserver tries to analyze the current file with Resyntax, see jeapostrophe/racket-langserver#153. This PR moves Resyntax's logic for handling such files out of the file group resolution code and into the core of `resyntax-analyze`.
@6cdh 6cdh force-pushed the add-resyntax-integration branch 2 times, most recently from 49b4645 to facb320 Compare July 27, 2025 09:54
@6cdh 6cdh force-pushed the add-resyntax-integration branch from facb320 to 9634ba6 Compare July 27, 2025 09:56
The new macro removes redundant `disable-resyntax!`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants