camlspotter / ocamloscope.2

OCamlOScope 2 : OCaml API search
42 stars 1 forks source link

Lots of double bindings #23

Closed camlspotter closed 7 years ago

camlspotter commented 7 years ago

I have noticed a lots of "double binding" warnings from Sigext. Is it 4.04 specific, or it has existed for 4.03? Probably this is the cause of having double entries of CamlP4.

camlspotter commented 7 years ago

The following code demonstrates how things are duped:

module type S = sig
  module M1 : sig
    type t = Bar
  end

  include module type of M1
end

Bar included by the include has the same stamp as the original Bar. Therefore S.t has a constructor S.M1.Bar instead of S.Bar.

This indicates Scan_ids and Globalized have some serious issues.

camlspotter commented 7 years ago

Changed t35_double_entry.ml:

module K = struct
  type s
end

module M1 = struct
  type t = Bar of K.s
end

module M2 : module type of M1 = struct
  type t = Bar of K.s
end

Though M1 and M2 share the same constructor T35_double_entry.M1.Bar wrongly, their types are different:

  ((KConstructor, "NOTOP.T35_double_entry.M1.Bar"),
   FVariantConstructor
     ("NOTOP.T35_double_entry.K.s -> NOTOP.T35_double_entry.M1.t", None));
  ((KConstructor, "NOTOP.T35_double_entry.M1.Bar"),
   FVariantConstructor
     ("NOTOP.T35_double_entry.K.s -> NOTOP.T35_double_entry.M2.t", None))
camlspotter commented 7 years ago

Method names can also crash with other names:

WARNING: double binding of input/1647 ({batteries}.BatIO.in_channel.input and {batteries}.BatIO.input)
WARNING: double binding of output/1652 ({batteries}.BatIO.out_channel.output and {batteries}.BatIO.output)
WARNING: double binding of flush/1654 ({batteries}.BatIO.out_channel.flush and {batteries}.BatIO.flush)

where for example,

type input = BatInnerIO.input

class in_channel : input ->
  object
    method input : string -> int -> int -> int
    method close_in : unit -> unit
  end

SInce a method name n is converted to Ident.t by Ident.create n, it can crash with other idents of name n.

This should be fixed by making Ident.t with minus stamp for methods...

camlspotter commented 7 years ago

Fixed (at least things I have found):

commit 8c9a9eca9ad371b7ea4c3c718d5e73f388a325c5
Author: Jun Furuse <Jun.Furuse@gmail.com>
Date:   Wed Nov 16 17:11:07 2016 +0000

    Fixed double binding issues of path names: variant constructors and record fields can be copied for other types. Method names are just strings in AST but they were converted to Ident.t which could crash with existing identifiers

commit 3799d376752689fa5ca7a4015515e71334df6831
Author: Jun FURUSE <jun.furuse@sc.com>
Date:   Wed Nov 16 19:30:17 2016 +0800

    fixing sigext

commit 7790c6bd80fc2142571915ab0c529f19031c8aeb
Author: Jun FURUSE <jun.furuse@sc.com>
Date:   Tue Nov 15 17:36:47 2016 +0800

    better examples to demonstrate double entry issue

commit b270a4a0e47d5a038cef0fc37902d8d23f56b56e
Author: Jun FURUSE <jun.furuse@sc.com>
Date:   Tue Nov 15 17:35:10 2016 +0800

    small fix for test
camlspotter commented 7 years ago

Some more fixes:

commit 17d4a6f9ad13faf996f1bcd7217ba5b50d914677
Author: Jun FURUSE <jun.furuse@sc.com>
Date:   Thu Nov 17 18:36:24 2016 +0800

    Fixed a bug around method identifiers which were misunderstood as predefiend

commit 4b9831b48371c1c06e40a74d42fd7b27997be612
Author: Jun FURUSE <jun.furuse@sc.com>
Date:   Thu Nov 17 16:00:05 2016 +0800

    cleanup: Sigext funcitons now take out_ident as the context path