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

Address most Emacs 29 warnings #3617

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Address most Emacs 29 warnings #3617

wants to merge 1 commit into from

Conversation

vemv
Copy link
Member

@vemv vemv commented Feb 1, 2024

Fixes #3606

Addresses indentation and other warnings.

Sadly we cannot immediately upgrade the CI linting, because:

  • Emacs 29 deprecates lax-plist-* functions
  • The alternative is based on an arity that isn't present in Emacs < 29.

Similarly, xref-go-back deprecates xref-pop-marker-stack, while the former only exists on Emacs 29, so there's no possible fix, AFAIK.

However, a great deal of warnings is now fixed, which should be less noisy to users, and pave the path future refinements.

Cheers - V

Sadly we cannot immediately upgrade the linting, because:

* Emacs 29 deprecates lax-plist-* functions
* The alternative is based on an arity that isn't present in Emacs < 28.
@vemv vemv requested a review from bbatsov February 1, 2024 20:17
symbol-name)))
(with-demoted-errors format
(cider--deep-vector-to-list (read indent))))
(with-demoted-errors "Some :indent metadata is unreadable! \nERROR: %s"
Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly the byte compiler kept complaining about the original shape of code, despite the many variations I tried.

I think it's ultimately because this linter assumes a string literal as an argument to with-demoted-errors.

So we have to give up on the informative string.

(push sym functions))
((and do-var (not is-function) (not is-macro))
(push sym vars)))))))))
(cl-labels ((handle-plist (plist)
Copy link
Member

Choose a reason for hiding this comment

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

The indentation here looks very wrong to me. If cl-labels is so broken in terms of indentation now I guess it'd be better to use a private function instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that this one looks odd, I couldn't produce any other accepted indentation.

it'd be better to use a private function instead.

👍

Copy link
Member

Choose a reason for hiding this comment

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

I'm always worried that if people edit the code on different versions of Emacs they'd get different indentations, which always results in a more painful contribution process. (e.g. I'm still using Emacs 28, as I didn't have a big incentive to move to Emacs 29)

Copy link
Member Author

Choose a reason for hiding this comment

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

In general, for thread-* and cl-labels I rearranged things in such a way that Emacs 29 and <29 will agree, which is why the PR's build remain green despite the indentation changes and no CI changes.

(for thread-*, it also happens to look more clojurey)

@@ -438,7 +438,7 @@ plugin or dependency with:
(defvar cider-version)

(defconst cider-manual-url "https://docs.cider.mx/cider/%s"
"The URL to CIDER's manual.")
"The URL to CIDER\='s manual.")
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that's not really a symbol.

Copy link
Member Author

Choose a reason for hiding this comment

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

One has to use it for all 's no matter what (which yes, looks fairly ugly)

https://emacs.stackexchange.com/posts/73048/revisions

@@ -451,7 +451,7 @@ plugin or dependency with:
(concat (cider-version-sans-patch) "/")))

(defun cider-manual-url ()
"The CIDER manual's url."
"The CIDER manual\='s url."
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@@ -474,7 +474,7 @@ to."
(buffer-string)))

(defconst cider-refcard-url "https://github.com/clojure-emacs/cider/raw/%s/refcard/cider-refcard.pdf"
"The URL to CIDER's refcard.")
"The URL to CIDER\='s refcard.")
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@bbatsov
Copy link
Member

bbatsov commented Feb 1, 2024

Similarly, xref-go-back deprecates xref-pop-marker-stack, while the former only exists on Emacs 29, so there's no possible fix, AFAIK.

Normally in such cases I copied stuff from the newer Emacs into cider-compat.el. (not sure if it currently exists) Backporting newer code is the only reasonable solution in such cases IMO.

I noticed that many of the changes are related to thread-first/last and cl-labels? Did they change their indentation settings? Seems like a pretty weird upstream change if so.

Copy link
Member Author

@vemv vemv left a comment

Choose a reason for hiding this comment

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

I noticed that many of the changes are related to thread-first/last and cl-labels? Did they change their indentation settings?

As far as I could try with Eldev (i.e. no personal settings involved), yes.

Normally in such cases I copied stuff from the newer Emacs into cider-compat.el. (not sure if it currently exists) Backporting newer code is the only reasonable solution in such cases IMO.

Yes, it was removed in c60598f

I could give it a shot. However we'd redefine lax-plist-* functions to receive an extra arity. That would seem a bit hacky and perhaps we'd be better off with the warnings.

We could upgrade the CI linter to emacs 29, but running a search/replace beforehand in the CI job for such cases? So that it doesn't complain for the cases that aren't reasonably fixable.

(push sym functions))
((and do-var (not is-function) (not is-macro))
(push sym vars)))))))))
(cl-labels ((handle-plist (plist)
Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that this one looks odd, I couldn't produce any other accepted indentation.

it'd be better to use a private function instead.

👍

@@ -438,7 +438,7 @@ plugin or dependency with:
(defvar cider-version)

(defconst cider-manual-url "https://docs.cider.mx/cider/%s"
"The URL to CIDER's manual.")
"The URL to CIDER\='s manual.")
Copy link
Member Author

Choose a reason for hiding this comment

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

One has to use it for all 's no matter what (which yes, looks fairly ugly)

https://emacs.stackexchange.com/posts/73048/revisions

@bbatsov
Copy link
Member

bbatsov commented Feb 25, 2024

I just saw this in the Emacs 29 changelog:

Indentation of 'cl-flet' and 'cl-labels' has changed.
These forms now indent like this:

    (cl-flet ((bla (x)
                (* x x)))
      (bla 42))

This change also affects 'cl-macrolet', 'cl-flet*' and
'cl-symbol-macrolet'.

(funcall pred var-meta))
items))))
(cl-labels ((keys-from-pred
(pred items)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should pull the params on the same line as the function name.

(when p
(goto-char p)
(message "%s" (get-char-property p 'cider-note))))))
(cl-labels ((goto-next-note-boundary ()
Copy link
Member

Choose a reason for hiding this comment

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

Weirdly this doesn't look like the indentation in the Emacs 29 changelog.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am using 29.2 and can confirm that the indentation is not correct here.

@p4v4n
Copy link
Contributor

p4v4n commented Feb 25, 2024

  • Emacs 29 deprecates lax-plist-* functions
  • The alternative is based on an arity that isn't present in Emacs < 29.

IIUC it's possible to just replace lax-plist-get/put with plist-get/put.
plist-get/put got an optional 3rd argument in emacs29+ but with 2 arguments they behave the same as lax-plist-*

(Edit: Not exactly the same but similar. lax-plist-get uses equal for comparison and plist-get uses eq. https://www.gnu.org/software/emacs/manual/html_node/elisp/Plist-Access.html)

Edit2: I was wrong. plist-* fns cannot be used as a direct replacement when the keys are strings.
Some alternatives solutions are using compat29.1 lib or replacing all plists to use keywords as keys.

@vemv
Copy link
Member Author

vemv commented Feb 25, 2024

Edit: Not exactly the same but similar. lax-plist-get uses equal for comparison and plist-get uses eq.

It may have been that prevented me from using a direct fix - I recall trying something and see the builds fail for E<29


I'll re-take this PR in March

@vemv vemv mentioned this pull request Jul 4, 2024
6 tasks
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.

Lots of compilation warnings since update to Emacs 29
3 participants