Rm extra indirection by making (color-identifiers:refontify) an alias

[Hi-Angel]

For Emacs 25.1 and higher that makes (color-identifiers:refontify) an alias to
(font-lock-flush). (color-identifiers:refontify) always was just a wrapper, so no
functional change is intended besides removing the unnecessary indirection.

I haven't found a way to just leave (if (fboundp … (defalias …) expression at the
top-level and not cause any byte-compilation errors, so the solution required a
little bit of hackery, but hopefully it should be safe. At least tests are passing
and I haven't found any problems.

[Hi-Angel]

@purcell since you're the author of the idea with aliasing font-lock (for hcl-mode):

(if (fboundp 'font-lock-flush)
    (defalias 'color-identifiers:refontify 'font-lock-flush)
  (defun color-identifiers:refontify ()
    (with-no-warnings (font-lock-fontify-buffer))))

I figured I'd CC you in case you have any solution. I tried implementing your idea, but it gives either this:

Error: the function ‘color-identifiers:refontify’ is not known to be defined.

or if I wrap it to (cl-eval-when 'compile that gives:

Error: the function ‘color-identifiers:refontify’ might not be defined at runtime.

So I ended up doing the hackery that can be seen in commit Rm extra indirection by making (color-identifiers:refontify) an alias. I'd like to avoid hacks of course, so Just wondering if you might know a proper solution to that.

Thanks in advance!

[Hi-Angel]

Per my understanding, in hcl-mode that wasn't a problem because actual calls to the alias were in different .el files. So when those files were getting compiled, the one with the alias was always loaded.

But here everything is in a single file, and I presume in this case splitting it just for the sake of making an alias not cause compilation errors would be an even larger hackery 😅

[purcell]

Trying to alias the not-yet-defined containing function is what confuses the byte compiler. I'd suggest:

(if (fboundp 'font-lock-flush)
    (defalias 'color-identifiers:refontify 'font-lock-flush)
  (defun color-identifiers:refontify ()
    "Refontify the buffer using font-lock.
    (with-no-warnings
      (and font-lock-mode (font-lock-fontify-buffer)))))

[Hi-Angel]

Aaah, actually, I just remember a workaround to that.

Your solution doesn't work, it still prints a color-identifiers-mode.el:684:4: Error: the function ‘color-identifiers:refontify’ is not known to be defined.. But the reason to that is this Emacs bug. And one workaround to that is adding a declare-function right before the defun. Like this.

[purcell]

Huh, weird. I see code with that pattern all over the place, but have not seen that compilation error before.

[Hi-Angel]

Well, maybe in other places the callers were in separate .el files? Because the error appears for the call-site, not the definition-site.

[purcell]

No, not at all. Mostly the same .el file. But looking again at this, you're apparently getting an error at a call site on line 684, but defining the function on a later line ~750, so of course the function is not known to be defined at the earlier call site. Just move the definition earlier in the file.

[Hi-Angel]

It still really doesn't, see (I also tried moving the code to the top, it doesn't influence anything):

[purcell]

Right, yeah, was testing while compiling multiple times in the same instance. I found another pattern I've used in the past btw:

(defalias 'windswap--swap-states
  (if (fboundp 'window-swap-states)
      'window-swap-states
    (lambda (a b)
      "Docstring for local function definition.
      ...)))

[purcell]

Just added CI to that project to confirm it compiles cleanly, and it does.

[Hi-Angel]

Oh wow, this one does work! And it's functional too, nice!

[purcell]

I probably found and solved that problem myself at some point in order to end up with that solution. :)

[purcell]

General observation here is that the current code in master is not well-packaged, since it advertises compatibility with Emacs 24, but the code actually requires at least Emacs 24.4. Given that, I'd suggest depending on Emacs 25.1, and then you don't need the defalias for font-lock-flush at all.

[Hi-Angel]

You might be right. Although, 24.4 is just a minor release of 24, but it indeed should be "24.4" then. And either way, 24.4 is 10 years old at this point (soon will be 11), it's very unlikely anybody's still using it. So might as well just drop the code.

OTOH, I am partly interested in the correct solution (disregarding if we drop Emacs 24) because I grepped over the code at my elpa/ dir and found a few more patterns like the one here. And I figured it might be an easy fix to send to other projects as well.

[Hi-Angel]

Well, running package-lint over the file shows that Emacs 24.x releases weren't so minor… Apparently they had lots of features added, and many of them are used in the mode.

So anyway, I added a commit correcting the version, to 24.4 for now. I tried integrating package-lint as well, but wasn't successful because from what I see the mode doesn't allow disabling some of the warnings — at least not the contains a non-standard separator ':' warning — and I definitely won't be able to rename identifiers as it's basically stable API.

[Hi-Angel]

and I definitely won't be able to rename identifiers as it's basically stable API.

That said — not that it matters given I can't rename it — but in my opinion using colons was a neat idea. Given that Emacs doesn't allow "local functions", so every function had to be prefixed with the plugin name to avoid naming clash, the colon allows to clearly separate that "workaround" part of the name from the actual function or variable name.

[purcell]

That said — not that it matters given I can't rename it — but in my opinion using colons was a neat idea.

It's cool to have an opinion or personal preference, and if Emacs were reinvented now perhaps things would be different, but the Emacs coding conventions explicitly establish two hyphens as the preferred separator for "private" symbols, and generally it is preferable to value community consistency over individual aesthetics. That's why package-lint is pedantic about this. I'm not actually sure how this package got into MELPA given its use of colons — we'd normally require package-lint issues to be fixed.

[purcell]

BTW, it is possible to migrate from : to -- and satisfy package-lint — the functions can be renamed, and then a define-obsolete-{function,variable}-alias can be used to keep the old name around.

[Hi-Angel]

BTW, it is possible to migrate from : to -- and satisfy package-lint — the functions can be renamed, and then a define-obsolete-{function,variable}-alias can be used to keep the old name around.

It's a lot of work, and then how long are we gonna keep obsolete names around, another ten years? Besides, obsoletion in plugins usually goes unnoticed. That's because to find out that you're using an obsolete variable/function you'd need to byte-compile a file, but many people don't byte-compile their .emacs config (because then you have to remember to re-compile it on every change. I was doing that when I first started using Emacs, but at some point just stopped.). So people wouldn't even know.

[Hi-Angel]

but the Emacs coding conventions explicitly establish two hyphens as the preferred separator for "private" symbols

Btw, these are conventions for code internal to Emacs, not for Elpa or plugins. I'm just saying this because that implies you can see "in the wild" code that doesn't accord to the conventions.

[purcell]

It's a lot of work, and then how long are we gonna keep obsolete names around, another ten years?

It's been done before in much more popular packages than this, so I can assure you it's perfectly possible.

But I really don't care here. It's not a great use of my time to engage further, sorry, just sharing my experience from reviewing thousands of packages as part of building up MELPA from 2012.

[Hi-Angel]

It's been done before in much more popular packages than this, so I can assure you it's perfectly possible.

There's a difference to changing/obsoleting a single variable/function, or doing that to everything in the entire plugin 😊

But I really don't care here. It's not a great use of my time to engage further, sorry, just sharing my experience from reviewing thousands of packages as part of building up MELPA from 2012.

It's okay, I understand.