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

manual: record and variant disambiguation #1647

Merged
merged 5 commits into from Mar 18, 2018

Conversation

Projects
None yet
4 participants
@Octachron
Copy link
Contributor

Octachron commented Mar 4, 2018

This PR adds a new subsection to the first chapter of the manual about the type-directed disambiguation of record fields and variant constructors just after the section on variant and records. This disambiguation is not currently documented in the manual, and tends to trip quite a few beginners.

Another emplacement that I considered was a new FAQ section at the end of the tutorial, but the new subsection seemed to fall quite well in place at its current location.

CC @Drup which suggested the subsection.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Mar 4, 2018

The writing is fine, but reading this explanation makes me deeply regret that the warning 41 (which fires off when the last visible label is chosen in absence of type information) is not enabled by default. This would be remove a lot of the "this doesn't look right" feeling of the current story.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Mar 5, 2018

I would gladly update the current subsection if warning 41 is enabled by default. I could also add some words of caution about relying on the last label word. However, I don't think that mentionning a warning not enabled by default in the tutorial chapter is a good idea.

type last_variant = A;;
\end{caml_example*}

The answer is that when confronted with multiple options, OCaml

This comment has been minimized.

@alainfrisch

alainfrisch Mar 6, 2018

Contributor

This might be a bit confusing, since the rest of the section explains why this sentence is incorrect. Also, I believe we should encourage people to rely on type annotation rather than on the order of definition to disambiguate references to labels/constructors.

What about explaining first that the locally available type information is used to disambiguate, and that the last defined label/constructor with the given name (in the current scope) is used as a fallback when no such information is available?

Also, it might be useful to explain that for record expressions and patterns, all fields are considered in the fallback (i.e. the last definition of a record type with all the listed fields is used).

This comment has been minimized.

@Octachron

Octachron Mar 7, 2018

Author Contributor

You are right reversing the order and exposing the type-directed disambiguation first and the fallback as a last resort yields a much more positive narrative than the current version.

Also, it might be useful to explain that for record expressions and patterns, all fields are considered in the fallback

I will also try to re-emphasize this part of the examples.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Mar 9, 2018

@gasche , I think that the new version should decrease the induced sense of dread

@garrigue

This comment has been minimized.

Copy link
Contributor

garrigue commented Mar 10, 2018

Looks nice to me too.

@gasche
Copy link
Member

gasche left a comment

It's much nicer, thanks. I made some minor comments.

fields or constructors share the same name

\begin{caml_example*}{toplevel}
type first = { x:int; y:int; z:int }

This comment has been minimized.

@gasche

gasche Mar 12, 2018

Member

You could use first_record for consistency with first_variant

This comment has been minimized.

@Octachron

Octachron Mar 12, 2018

Author Contributor

Good idea.

For instance:

\begin{caml_example}{toplevel}
let look_at_x_then_y_then_z (r:first) =

This comment has been minimized.

@gasche

gasche Mar 12, 2018

Member

The function name doesn't match the implementation anymore?

This comment has been minimized.

@Octachron

Octachron Mar 12, 2018

Author Contributor

You are right, fixed.

let look_at_x_then_y_then_z (r:first) =
let x = r.x in
x + r.z;;
let rotate_abc_to_bac (x:first_variant) = match x with

This comment has been minimized.

@gasche

gasche Mar 12, 2018

Member

mayberotate would be enough

This comment has been minimized.

@Octachron

Octachron Mar 12, 2018

Author Contributor

I went with permute.

\end{caml_example}

Consequently, adding explicit type annotations to guide disambiguation is
more robust than relying on the last defined type disambiguation.

This comment has been minimized.

@gasche

gasche Mar 12, 2018

Member

Another thing to point out is that disambiguation using last declaration is not robust to introducing new declarations or opening modules, which means that the disambiguation choice may change as the code evolve, unlike disambiguation based on type information.

This comment has been minimized.

@Octachron

Octachron Mar 12, 2018

Author Contributor

I added a short paragraph on this matter.

@gasche

gasche approved these changes Mar 12, 2018

Copy link
Member

gasche left a comment

I like the current state so I approved the PR. I am not merging right now in case other people have other polishing comments to make. @Octachron, feel free to merge whenever. Also, it could be nice to cherry-pick in 4.06 as well -- I recently cherry-picked a manual typo there, in case we decide to re-upload a copy of the documentation.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Mar 13, 2018

I will merge by the end of the week to give some time for comments.

@Octachron Octachron merged commit 6d65f78 into ocaml:trunk Mar 18, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Mar 18, 2018

Merged, I will cherry-pick to 4.06 tomorrow.

Octachron added a commit that referenced this pull request Mar 19, 2018

manual: record and variant disambiguation (#1647)
This commit adds a subsection to the record and variant section of the manual describing the behavior of the type-directed disambiguation of variant constructors and record fields.
@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Mar 19, 2018

Cherry-picked in 4.06 as 6a7a46f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.