Skip to content

tries to make more meaningful names for the general types#882

Closed
SteveBronder wants to merge 2 commits intostan-dev:masterfrom
SteveBronder:general-type-names
Closed

tries to make more meaningful names for the general types#882
SteveBronder wants to merge 2 commits intostan-dev:masterfrom
SteveBronder:general-type-names

Conversation

@SteveBronder
Copy link
Copy Markdown
Contributor

@SteveBronder SteveBronder commented Apr 15, 2021 •
edited
Loading

Summary

When I'm working over the ocaml it's pretty often I'll run into some type like ('a, 'b) Stmt.Fixed... and I always found it kind of harder to parse out what 'a or 'b is. This PR tries to give "nicer" names for those general types.

I get that yes 'a is just a general type, but in the types they are used, for example in Stmt.Fixed.Pattern the general types ('a ,'b) on the pattern t is not going to be like a string, websocket, or a random number generator. The general type there is going to be some type that is derived or has inside of it expressions. So why don't we just use a nicer name like

  module Pattern = struct
    type ('expr, 'stmts) t =
      | Assignment of 'expr lvalue * 'expr
      | TargetPE of 'expr
      | NRFunApp of Fun_kind.t * 'expr list
      | Break
      | Continue
      | Return of 'expr option
      | Skip
      | IfElse of 'expr * 'stmts * 'stmts option
      | While of 'expr * 'stmts
      | For of {loopvar: string; lower: 'expr; upper: 'expr; body: 'stmts}
      | Profile of string * 'stmts list
      | Block of 'stmts list
      | SList of 'stmts list
      | Decl of
          { decl_adtype: UnsizedType.autodifftype
          ; decl_id: string
          ; decl_type: 'expr Type.t }
    [@@deriving sexp, hash, map, fold, compare]

What I'd like is for the names of the types to be the least upper bound of the types that we expect to go into the type. Here I just looked around and sorted out that the 'a is going to be a thing that holds an expression, where 'b is generally a thing that holds a list of statements. imo I think this is a lot easier to read when I'm looking at types elsewhere in the program.

Though I think there is some places I have bad names rn. Like in expr_with idt 'fun_kind is the right name for the least upper bound of types that can go in there since an untyped_expression is an expr_with that takes in a unit.

type ('meta_loc, 'fun_kind) expr_with =
  { expr: (('meta_loc, 'fun_kind) expr_with, 'fun_kind) expression
  ; emeta: 'meta_loc }
[@@deriving sexp, compare, map, hash, fold]

(*later...*)
type untyped_expression = (located_meta, unit) expr_with
[@@deriving sexp, compare, map, hash, fold]

Maybe just 'any is more appropriate when the type can be a unit?

Expressive names that are wrong are certainly worse than meaningless names, but I think if someone can look over the names I have here and help me sort out the weird ones then this should be a bit nicer to work over

Release notes

Cleanup names of generic types inside of type declarations

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

@nhuurre
Copy link
Copy Markdown
Collaborator

nhuurre commented Apr 16, 2021

I like this but it doesn't seem to affect VSCode tooltips.
vscode-tooltip

Like in expr_with idt 'fun_kind is the right name for the least upper bound of types that can go in there since an untyped_expression is an expr_with that takes in a unit.

'fun_kind is correct because the type variable represents the function kind. Untyped AST has only one kind of function and the kind is represented by a unit but it's still a kind field.

I think you should drop the s from 'stmts so it's just 'stmt. It's the type of a single statement. A list of statements is usually wrapped in a Block statement. | Block of has list after the type.

Program.t should be ('expr, 'stmt) t as the first type is the output dimension type, same as SizedType.t's 'dim_expr.

@SteveBronder
Copy link
Copy Markdown
Contributor Author

SteveBronder commented Apr 16, 2021

Oof the vs code thing, lemme look at it. It seems like sometimes it would work for me. Maybe something goofy is going on.

Agree with the rest tho'!

@SteveBronder
Copy link
Copy Markdown
Contributor Author

SteveBronder commented Apr 16, 2021

So I'm pretty sure it's that we are on an old version of ocaml so there's not much we can do about it. The newer extensions might fix this, but those require the lsp server which requires dune 2.5 which requires ocaml > 4.07 :-(

I tried getting a branch up and running that used ocaml 4.11.1 (which rok said is the latest we can use because of windows things) but I'm getting too many goofs and I'm not sure how to resolve them

https://github.com/SteveBronder/stanc3/tree/update/4.11.1

@SteveBronder
Copy link
Copy Markdown
Contributor Author

SteveBronder commented Apr 20, 2021

Ugh so yeah it has to do with the ocaml compiler version, lsp-server, and the vscode ocaml extension. I hacked at #885 to get it to work for 4.12.0 (which we can't use yet because the windows cross compiler only supports 4.11.1). I still can't get the nice names to show up for id_of_value but I can get the nice names to show up at the call site, which is really weird??

@WardBrian WardBrian mentioned this pull request Nov 1, 2021
Merged
11 tasks
@WardBrian
Copy link
Copy Markdown
Member

WardBrian commented Dec 13, 2021

@SteveBronder do you want to take another crack at this sometime?

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@SteveBronder @nhuurre @WardBrian