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

more -dsource bug fix #915

Merged
merged 4 commits into from Dec 19, 2016

Conversation

Projects
None yet
4 participants
@objmagic
Contributor

objmagic commented Nov 14, 2016

This PR is a follow-up of #539, which aims to fix two more corner case -dsource(pprintast.ml) bugs.

Cf. Mantis 7200

Bug 1: GADT exp

Parse tree of let f: type a'.a' = assert false is incorrectly printed to let f : 'a' . 'a' = fun (type a') -> (assert false : a'). This bug happens since a' is a valid new type name according to lexing rule

Solution: really pretty-print GADT let-expression by restoring the desugared GADT let-expression. After this PR, the above example will have output let f : type a'.a' = assert false

Bug 2: Polymorphic method

Parse tree of

class ['a] c () = object
  constraint 'a = < .. > -> unit
  method m  = (fun x -> () : 'a)
end

is incorrectly printed to

class ['a] c () =
  object constraint 'a = < .. > -> unit method m : 'a = fun x  -> () end

This is wrong because now two classes have different signatures. The input has signatures

class ['a] c : unit ->
  object constraint 'a = (< .. > as 'b) -> unit method m : 'b -> unit end

but the output has

class ['a] c :
    unit -> object constraint 'a = (< .. > as 'b) -> unit method m : 'a end

Solution: delete the part that incorrectly optimizes pretty-printing in this situation.

-    | Pexp_constraint (e,t1),Ppat_var {txt;_} ->
-      pp f "%a@;:@ %a@;=@;%a" protect_ident txt
-        (core_type ctxt) t1 (expression ctxt) e

cc @alainfrisch

@@ -73,6 +73,8 @@ module Typ :
val extension: ?loc:loc -> ?attrs:attrs -> extension -> core_type
val force_poly: core_type -> core_type
val varify_constructors: str list -> core_type -> core_type

This comment has been minimized.

@alainfrisch

alainfrisch Nov 14, 2016

Contributor

It it worth documenting what this function does since/if this function is put in such a "rather public" part of compiler-libs.

@alainfrisch

alainfrisch Nov 14, 2016

Contributor

It it worth documenting what this function does since/if this function is put in such a "rather public" part of compiler-libs.

This comment has been minimized.

@objmagic

objmagic Nov 15, 2016

Contributor

I added documentation here. Since my knowledge about type checker is limited, please feel free to point out any word that needs revision.

@objmagic

objmagic Nov 15, 2016

Contributor

I added documentation here. Since my knowledge about type checker is limited, please feel free to point out any word that needs revision.

This comment has been minimized.

@objmagic

objmagic Nov 15, 2016

Contributor

IIRC, @gasche added this function. Could you please check if the documentation I wrote is accurate? Thanks.

@objmagic

objmagic Nov 15, 2016

Contributor

IIRC, @gasche added this function. Could you please check if the documentation I wrote is accurate? Thanks.

This comment has been minimized.

@gasche

gasche Nov 15, 2016

Member

Hi @objmagic :-) The function was added by Jacques Le Normand in 2010: 90a6348

@gasche

gasche Nov 15, 2016

Member

Hi @objmagic :-) The function was added by Jacques Le Normand in 2010: 90a6348

@@ -94,6 +94,8 @@ Next version (4.05.0):
list in .cmti files + avoid rebuilding cmi_info record when creating
.cmti files
(Alain Frisch, report by Daniel Bunzli, review by Jeremie Dimino)
- GPR#915: fix -dsource (pprintast.ml) bugs
(Runhang Li, review by Alain Frisch)

This comment has been minimized.

@alainfrisch

alainfrisch Nov 14, 2016

Contributor

I appreciate the gentle hint :)

@alainfrisch

alainfrisch Nov 14, 2016

Contributor

I appreciate the gentle hint :)

Show outdated Hide outdated parsing/ast_helper.mli
@@ -73,6 +73,15 @@ module Typ :
val extension: ?loc:loc -> ?attrs:attrs -> extension -> core_type
val force_poly: core_type -> core_type
val varify_constructors: str list -> core_type -> core_type
(** [varify_constructors newtypes tc] is type constructor [tc], of which

This comment has been minimized.

@alainfrisch

alainfrisch Nov 15, 2016

Contributor

It seems the second argument tc can be an arbitrary type expression, no?

@alainfrisch

alainfrisch Nov 15, 2016

Contributor

It seems the second argument tc can be an arbitrary type expression, no?

This comment has been minimized.

@objmagic

objmagic Nov 15, 2016

Contributor

Thanks. I have updated the documentation. Please check again.

@objmagic

objmagic Nov 15, 2016

Contributor

Thanks. I have updated the documentation. Please check again.

objmagic added some commits Jul 20, 2016

Raise [Syntaxerr.Variable_in_scope] if any type variable inside [te]
appears in [newtypes].
@since 4.05
*)

This comment has been minimized.

@Octachron

Octachron Nov 15, 2016

Contributor

May I suggest a slightly different wording?

(** 
    [varify_constructors newtypes te] is the type expression [te'], 
     where all nullary type constructors whose name appears in [newtypes]
     have been replaced by a type variable of the same name.
     Raise [Syntaxerr.Variable_in_scope] if any type variable inside [te]
     appears in [newtypes].
     @since 4.05
     *)

N.B: Please feel free to ignore or improve upon this suggestion.

@Octachron

Octachron Nov 15, 2016

Contributor

May I suggest a slightly different wording?

(** 
    [varify_constructors newtypes te] is the type expression [te'], 
     where all nullary type constructors whose name appears in [newtypes]
     have been replaced by a type variable of the same name.
     Raise [Syntaxerr.Variable_in_scope] if any type variable inside [te]
     appears in [newtypes].
     @since 4.05
     *)

N.B: Please feel free to ignore or improve upon this suggestion.

This comment has been minimized.

@objmagic

objmagic Nov 16, 2016

Contributor

Thank you but I feel it makes very few difference.

@objmagic

objmagic Nov 16, 2016

Contributor

Thank you but I feel it makes very few difference.

@objmagic

This comment has been minimized.

Show comment
Hide comment
@objmagic

objmagic Nov 18, 2016

Contributor

This should be good to go.

Contributor

objmagic commented Nov 18, 2016

This should be good to go.

@objmagic

This comment has been minimized.

Show comment
Hide comment
@objmagic

objmagic Dec 19, 2016

Contributor

any further feedback on this PR? I believe it should be good to go. @alainfrisch

Contributor

objmagic commented Dec 19, 2016

any further feedback on this PR? I believe it should be good to go. @alainfrisch

@alainfrisch alainfrisch merged commit 807928c into ocaml:trunk Dec 19, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 19, 2016

Contributor

Thanks for the reminder. Looks good to me. Merged!

Contributor

alainfrisch commented Dec 19, 2016

Thanks for the reminder. Looks good to me. Merged!

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment