Skip to content

Conversation

setton
Copy link
Member

@setton setton commented Mar 10, 2021

For now, we do not support an actual preprocessor, but work
in a degraded mode where the "#if" branch of preprocessing is
always chosen.

This allows responding to requests in cases where simply
"ignoring all preprocessing" is not sufficient because it
results in non-valid syntax, for instance

 #if A procedure foo ( # else procedure foo ( # endif 

We do this by intercepting buffers before they are sent
to Libadalang, and preprocess on-the-fly.

This also adds support for working with non-LF line terminators:
the line terminators are changed to LF as part of the
preprocessing.

@setton setton force-pushed the topic/preprocessing branch from 0cbd919 to 231a768 Compare March 10, 2021 18:30
For now, we do not support an actual preprocessor, but work in a degraded mode where the "#if" branch of preprocessing is always chosen. This allows responding to requests in cases where simply "ignoring all preprocessing" is not sufficient because it results in non-valid syntax, for instance #if A procedure foo ( # else procedure foo ( # endif We do this by intercepting buffers before they are sent to Libadalang, and preprocess on-the-fly. This also adds support for working with non-LF line terminators: the line terminators are changed to LF as part of the preprocessing.
@setton setton requested a review from reznikmm March 14, 2021 21:14
Copy link
Member

@reznikmm reznikmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why do we need to change line terminator? I'm afraid this could confuse the client. If server sends '\n' instead of actual line ending in the workspace edit request, the source could become a mess on the client side.
  2. Also I'm not sure if the ALS should do source preprocessing instead of LAL. With this approach any server side modifications will not work. For instance
    • Format document/format selection will not work, because server will send empty lines instead of #els parts and directives. So client will just clean these line up.
    • Refactoring will not work inside #els parts
  3. I'm not sure that incremental text updates from the client (in #els parts) won't break server, because server won't find text position from the notification.
elsif Line.Starts_With (To_Virtual_String ("#el")) then
This_branch_Evaluates_To_True := Eval (Line);
elsif Line.Starts_With (To_Virtual_String (("#end"))) then
Currently_Preprocessing := False;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes #end appears in the Result and this isn't what we want, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but actually LAL simply ignores it. I'll change so that it's clearer.

@setton
Copy link
Member Author

setton commented Mar 17, 2021

  1. Why do we need to change line terminator? I'm afraid this could confuse the client. If server sends '\n' instead of actual line ending in the workspace edit request, the source could become a mess on the client side.

I was thinking that we need to change the line terminators before sending to GS, since GS converts any file from disk to using LF whilst in buffer. But in fact I will move this part to the client, this seems to be what's expected by the server - at least that's my interpretation of this new field in the WorkspaceEditCapabilities

 /** * Whether the client normalizes line endings to the client specific * setting. * If set to `true` the client will normalize line ending characters * in a workspace edit to the client specific new line character(s). * * @since 3.16.0 */ normalizesLineEndings?: boolean; 

So, I'll make the change.

  1. Also I'm not sure if the ALS should do source preprocessing instead of LAL. With this approach any server side modifications will not work. For instance

    • Format document/format selection will not work, because server will send empty lines instead of #els parts and directives. So client will just clean these line up.
    • Refactoring will not work inside #els parts

Agreed but unfortunately LAL has no plan right now to support source preprocessing. We can keep discussing this with them in the TN but we need something to fix the customer's case.

Right now LAL ignores completely any preprocessing directive, so this patch is an improvement in that at least it will allow users to navigate in the presence of refactoring that breaks the syntax.

  1. I'm not sure that incremental text updates from the client (in #els parts) won't break server, because server won't find text position from the notification.

This should work: what we're doing is intercepting the buffer before it gets sent to LAL, but the support of incremental text is upstream from this.

@setton setton closed this Mar 17, 2021
@setton setton reopened this Mar 17, 2021
@reznikmm
Copy link
Member

I will move this part to the client,
We need it on the client side anyway, to protect GS from clangd (and potential others servers).

@setton
Copy link
Member Author

setton commented Mar 17, 2021

following up under #640

@setton setton closed this Mar 17, 2021
@setton setton deleted the topic/preprocessing branch March 18, 2021 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants