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

"tuareg-eval-phrase" Error with class type #239

Closed
Fourchaux opened this issue Sep 7, 2020 · 7 comments
Closed

"tuareg-eval-phrase" Error with class type #239

Fourchaux opened this issue Sep 7, 2020 · 7 comments

Comments

@Fourchaux
Copy link

Fourchaux commented Sep 7, 2020

OCaml 4.11.1 - Tuareg 2.2.0 - The Merlin toolkit v3.3.9, for OCaml 4.11.1 - Emacs 28.0.50

Example from https://caml.inria.fr/pub/docs/manual-ocaml-4.11/objectexamples.html#s:class-interfaces

class type restricted_point_type =
  object
    method get_x : int
    method bump : unit
  end;;
  • Error with "tuareg-eval-phrase" :
Line 1, characters 5-7:
1 | class;;
         ^^
Error: Syntax error
  • OK with "tuareg-eval-region" :
class type restricted_point_type =
  object method bump : unit method get_x : int end
mattiase added a commit to mattiase/tuareg that referenced this issue Jun 24, 2021
Lex "type" after "class" as "c-type", in analogy with "m-type".
@mattiase
Copy link
Contributor

This patch seems to work. Drawback: the indentation of your example is not preserved; that is,

class type restricted_point_type =
  object
    method get_x : int
    method bump : unit
  end

is indented as

class type restricted_point_type =
object
  method get_x : int
  method bump : unit
end

which corresponds to the indentation of class definitions:

class c =
object
  method m = 1
end

On the other hand, putting object at the end of the first line (analogously to module type M = sig...) now produces decent indentation:

class type restricted_point_type = object
  method get_x : int
  method bump : unit
end;;

which was not the case before.

In other words: the patch fixes the class type parsing. Is the indentation acceptable? If not, what is the desired style? (I don't use classes much but would probably prefer the object-on-the-first-line style myself.)

mattiase added a commit to mattiase/tuareg that referenced this issue Jun 24, 2021
Lex "type" after "class" as "c-type", in analogy with "m-type".
@Fourchaux
Copy link
Author

No more 'Syntax error' : well done.

Note: I don't have your indentation problem.
(for info I'm using ocp-indent)

So, for me, this issue can be closed.
Thank you very much.

@mattiase
Copy link
Contributor

Thanks for confirming, and yes, ocp-indent uses different rules. It appears that tuareg's class indentation,

class c =
object
  method m = ()
end

is very much intended (see bcad60e) but it's unclear to what extension it describes a consensus among OCaml programmers. The OCaml source tree itself is inconsistent in this respect, but that is not unexpected for a long-lived code base with many contributors.

It looks like the indentation consequences of the patch are neutral at worst, and fixing the parsing bug is worth it.

@monnier
Copy link
Contributor

monnier commented Jun 25, 2021 via email

@mattiase
Copy link
Contributor

We could also make it so that we get:

class c =
  object
    method m = ()
  end

but still keep

class c = object
  method m = ()
end

Yes, that would coincide with ocp-indent – I think that would be preferable, and class type should be laid out the same way. However, I'm not sure what the best way to accomplish that would be; SMIE indentation code is still somewhat of a mystery to me.

It looks like the indentation consequences of the patch are neutral at
worst, and fixing the parsing bug is worth it.

And it shouldn't be terribly hard to change the indentation separately
if needed.

This patch OK then?

mattiase added a commit to mattiase/tuareg that referenced this issue Jun 27, 2021
Lex "type" after "class" as "c-type", in analogy with "m-type".
mattiase added a commit to mattiase/tuareg that referenced this issue Jun 27, 2021
Indent non-hanging `object` instead of aligning with their preceding
`class` token. Previously:

class c =
object
  method m = 1
end

Now:

class c =
  object
    method m = 1
  end

which agrees with default ocp-indent and seems to be the more modern
usage. Also indent `initialize` clauses correctly. (ocaml#239)
@mattiase
Copy link
Contributor

This should now be fixed; close if you agree.

@Fourchaux
Copy link
Author

Done.

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

No branches or pull requests

3 participants