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

Fix #757 indentation of "\in" #789

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

Conversation

Matafou
Copy link
Contributor

@Matafou Matafou commented Sep 10, 2024

The lexer now glues a "" to an immediately following word if it is itself preceded by a space.

Note that for really good indentation you should try to add something like this to your configuration:

(setq coq-smie-user-tokens '(("\in" . "=")))

to tell the indentation grammar that \in has the same precedence as "=".

@Matafou
Copy link
Contributor Author

Matafou commented Sep 10, 2024

The problem was that \in was lexed as "" "in" and that "in" is a (pretty crowdedly overloaded) reserved keyword for let in, Let in,..

I will wait a feedback from #757 before merging this.

Copy link
Member

@erikmd erikmd left a comment

Choose a reason for hiding this comment

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

Hi @Matafou, thanks a lot for this fix!

I tested it successfully on another file of mine, as well as on your test file and it worked-out-of-the-box™ (using Emacs 28.2 on Debian 12)

ci/test-indent/test_backslash_ops.v Outdated Show resolved Hide resolved
ci/test-indent/test_backslash_ops.v Outdated Show resolved Hide resolved
@erikmd
Copy link
Member

erikmd commented Sep 10, 2024

@Matafou Thanks a lot for this fix, which will be important for ssreflect & mathcomp users.

I'd say that maybe, this fix + your previous fix #782 alone (!) would deserve to prepare a release soonish.
(The main missing ingredient would be an up-to-date CHANGES file.
Then maybe also some cleanup of the make release command, taking into account this comment.)

@hendriktews @Matafou WDYT?

@erikmd erikmd added kind: fix part: indentation Problems with indentation feature of PG labels Sep 10, 2024
The lexer now glues a "\" to an immediately following word if it is
itself preceded by a space.

Note that for really good indentation you should try to add something
like this to your configuration:

(setq coq-smie-user-tokens '(("\\in" . "=")))

to tell the indentation grammar that \in has the same precedence as
"=".

Test added.
@Matafou Matafou force-pushed the fix-indent-backslashid branch 2 times, most recently from 8e276e6 to a36eb9b Compare September 11, 2024 08:08
@Matafou
Copy link
Contributor Author

Matafou commented Sep 11, 2024

I don't know the exact use of \xxx in mathcomp, are all these \symbols non associative infix operators?

Note that without the coq-smie-user-tokens trick these tokens will be considered with a default rule, precedence and associativity (@monnier can you tell what are these for a token not appearing in the rules and grammar, starting with a \). This can lead to strange indentation maybe. @vzaliva tell us if it is the case it should be easy to fix.

@erikmd
Copy link
Member

erikmd commented Sep 12, 2024

I don't know the exact use of \xxx in mathcomp,

Basically, the default notation convention in the mathcomp core library is to only use ASCII operators, not UTF-8 or so, and typically it relies on notations very close to those from LaTeX. E.g.:

are all these \symbols non associative

not exactly, cf. the \o operator for function composition.

infix operators?

You also have \sum_i, \prod_i, \bigcap_i, \bigcup_i and a few others.

IIUC your PR code, the fix is not specific to \in 👍
so it should also work for the \notin and \o infixes for example (I didn't test)

@Matafou
Copy link
Contributor Author

Matafou commented Sep 12, 2024

OK, I was hoping for something simple. How naive I am :-).

If we can come up with a decent coq-smie-user-tokens declaration for usual ssreflect operators we could put them into coq-standard-token-synonyms.

It would look like:

(setq coq-smie-user-tokens '(("\in" . "=") ("\notin" . "=") ("\o" . "+") ... )) 

(I guess \sum() is lexed/parsed as a standard function application and that's ok).

Maybe this already exists somewhere?

@Matafou
Copy link
Contributor Author

Matafou commented Sep 12, 2024

Yes the fix is for any token starting with \. But indentation is not the same for a infix associative operator and a prefix one.

@erikmd
Copy link
Member

erikmd commented Sep 14, 2024

OK thanks!

So IIUC, in addition to this PR, it just suffices to provide a relevant (setq coq-smie-user-tokens '(…)) list so indenting ssreflect would work out-of-the-box?

BTW I tested (setq coq-smie-user-tokens '(("\in" . "=")) vs. (setq coq-smie-user-tokens '(("\in" . "+")) and didn't notice a specific change in the indentation; could you help by providing a minimal complete example where the two infixes are indented differently?

@Matafou
Copy link
Contributor Author

Matafou commented Sep 19, 2024

The difference can be subtle and this is very fragile, but here is a difference between + and = (that would reflect on \in):

Lemma foo:
  forall x y,
    y
    =
      z
    = y.

Lemma foo:
  forall x y,
    y
    =
      z
      + y.

@Matafou
Copy link
Contributor Author

Matafou commented Sep 19, 2024

Note that this is a bit wrong since = is not associative, but that is the idea.

@erikmd
Copy link
Member

erikmd commented Sep 19, 2024

OK thanks @Matafou !

So I'll try to come up with a comprehensive list of the \in | \notin | … mathcomp symbols this WE.

Question: can/should this PR be merged beforehand? or implementing the coq-standard-token-synonyms list you mentioned will require a non-trivial refactoring of this PR's code?

@Matafou
Copy link
Contributor Author

Matafou commented Sep 20, 2024

I think we can merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: fix part: indentation Problems with indentation feature of PG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants