Skip to content
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

Native json support #691

Merged
merged 1 commit into from
Mar 14, 2019
Merged

Native json support #691

merged 1 commit into from
Mar 14, 2019

Conversation

yyoncho
Copy link
Member

@yyoncho yyoncho commented Mar 4, 2019

Fixes #210
Fixes #395

  • Ajusted lsp-mode to use native json parsing
  • introduced 2 new flags:

** lsp-use-native-json
** lsp-json-use-lists - could be used to switch to the list serialization in case
there is a problem with the vector serialization.

  • Addressed few bottlenecks in the code.

Copy link
Contributor

@hpdeifel hpdeifel left a comment

Choose a reason for hiding this comment

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

#690 is indeed fixed with these changes.

lsp-mode.el Outdated
(end (if (stringp selected-param-label)
(+ start (length selected-param-label))
(seq-elt selected-param-label (1- (seq-length selected-param-label))))))
(last (append selected-param-label nil)))))
Copy link
Contributor

Choose a reason for hiding this comment

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

last returns a cons cell. What's wrong with elt?

Copy link
Member Author

Choose a reason for hiding this comment

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

it should be cl-second

lsp-mode.el Outdated
(json-parse-string str
:object-type 'hash-table
:null-object nil
:false-object :json-false)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Switching from nil to :json-false leads to conditionals with json values to suddenly succeed. For example, (gethash "resolveProvider" ...) in lsp--sort-completions now returns a truth-value (:json-false) for ccls, leading to errors during completion. There may be other similar instances.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for spotting this. We had to evaluate whether to keep the current(incorrect) behaviour of fix all places which do if over a lsp result.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for :false/:null, as in the defaults of the emacs27 json library, or even nil instead of :false, that way you preserve truth value.

Copy link
Member Author

Choose a reason for hiding this comment

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

We already use :json-false for false when doing serialization.

Copy link
Member

Choose a reason for hiding this comment

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

But that doesn't affect us much as we don't reuse the objects we create during serialization. On the other hand, using a non-nil value while working with values we get from the language server would inadvertently result in a lot of broken code. Every if and conditional construct would have to be rewritten to check for the new "false", which would just result in inorganic and unidiomatic lisp code, prone to breaking.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vibhavp there are few places we reuse the object, e. g. - lenses, diagnostics, code actions.

which would just result in inorganic and unidiomatic lisp code, prone to breaking.

I agree that it will be ugly, error-prone, difficult to migrate and support. And I agree with you that it is better to keep it as it is at least for now...

A link to a code that deals with that issue - I am converting the nil to :json-false to make it work.
https://github.com/emacs-lsp/lsp-java/blob/master/lsp-java-boot.el#L68

@hpdeifel
Copy link
Contributor

hpdeifel commented Mar 5, 2019

By the way, is lsp-json-use-lists intended to go away at some point? Why do you do all this seq generic stuff instead of committing to vectors?

@yyoncho
Copy link
Member Author

yyoncho commented Mar 5, 2019

By the way, is lsp-json-use-lists intended to go away at some point?

Yes, we might not add it at all.

Why do you do all this seq generic stuff instead of committing to vectors?

IMO using seq will make the code more robust. I am not sure whether there is vector-filter or similar functions or we should resort to smt like cl-loop.

Edit: but it is still an option that we should evaluate.

Fixes #210
Fixes #395

* Ajusted lsp-mode to use native json parsing
* introduced 2 new flags:

** lsp-use-native-json
** lsp-json-use-lists - could be used to switch to the list serialization in case
  there is a problem with the vector serialization.

* Addressed few bottlenecks in the code.
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.

None yet

3 participants