From be87559ef39ce11adb2969fa8dc4f1f91199a373 Mon Sep 17 00:00:00 2001 From: Adam Gundry Date: Fri, 25 Sep 2020 10:48:26 +0100 Subject: [PATCH 1/6] DuplicateRecordFields without ambiguous selectors --- proposals/0000-no-ambiguous-selectors.rst | 184 ++++++++++++++++++++++ 1 file changed, 184 insertions(+) create mode 100644 proposals/0000-no-ambiguous-selectors.rst diff --git a/proposals/0000-no-ambiguous-selectors.rst b/proposals/0000-no-ambiguous-selectors.rst new file mode 100644 index 0000000000..0d779ae64d --- /dev/null +++ b/proposals/0000-no-ambiguous-selectors.rst @@ -0,0 +1,184 @@ +DuplicateRecordFields without ambiguous selectors +================================================= + +.. author:: Adam Gundry +.. date-accepted:: Leave blank. This will be filled in when the proposal is accepted. +.. ticket-url:: Leave blank. This will eventually be filled with the + ticket URL which will track the progress of the + implementation of the feature. +.. implemented:: Leave blank. This will be filled in with the first GHC version which + implements the described feature. +.. highlight:: haskell +.. header:: This proposal is `discussed at this pull request `_. +.. contents:: + +This proposal addresses an unsatisfactory aspect of ``DuplicateRecordFields``, namely the unclear rules around when a field selector or update will be accepted, by entirely removing the type-directed name resolution aspect. This proposal is +more restrictive than the `previous (dormant) simplification proposal 84 `_, allowing fewer programs, but correspondingly simpler. + + +Motivation +---------- +The ``DuplicateRecordFields`` extension is useful to allow multiple datatypes to be defined with the same field name in the same module, for example:: + + module M where + + data Person = MkPerson { name :: String, pets :: [Pet] } + data Pet = MkPet { name :: Text } + +It is now entirely unproblematic to use ``name`` in a record construction or pattern match, because the constructor name disambiguates which field is meant:: + + simon = MkPerson { name = "Simon" } + + getPetName (MkPet {name = x }) = x + +However a bare use of the selector is more troublesome. Given the definition ``getName x = name x``, is ``x`` a ``Person`` or a ``Pet``? Similar issues arise for record updates. + +At present, potential ambiguities are sometimes resolved by the type-checker, using bidirectional type-checking. However, this works only in rather limited circumstances, has a complicated implementation, and has proved confusing for users. We therefore propose to simplify the extension by removing the possibility for the type-checker to resolve ambiguities. This would mean that fewer programs are accepted, but would simplify both the specification and the implementation of ``DuplicateRecordFields``. + + +Proposed Change Specification +----------------------------- +A "field selector occurrence" is an in-scope identifier occurring in an expression, for which *all* the entities to which it could refer are record fields, e.g. ``name`` in ``getName x = name x`` with the definitions above. A "field update occurrence" is an in-scope identifier occurring on the left-hand side of an equals sign in a record update expression, e.g. ``name`` in ``e { name = z }``. A field selector or field update occurrence is "ambiguous" if it refers to more than one field in scope (i.e. there are multiple results when the identifier is looked up during name resolution). + +When ``DuplicateRecordFields`` is disabled, ambiguous field selector/update occurrences are currently rejected during name resolution. However, when it is enabled, they are currently disambiguated by the type-checker using the rules described below. This proposal suggests removing most of these rules, and instead rejecting such definitions during name resolution. + +Note that if an identifier refers to both non-fields and fields, it is always rejected by the renamer as excessively ambiguous, regardless of ``DuplicateRecordFields``. + + +Existing rules for selectors +^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +The rules for resolving ambiguous field selectors occurrences (i.e. those for which multiple fields with the same label are in scope) are currently as follows: + +1. If the selector is applied to an argument, and there is a type signature on the argument which determines a datatype, use that datatype. + +2. During bidirectional type-checking, if the type being pushed in is a function whose domain determines a datatype, use that datatype. + +(The meaning of "determines a datatype" is not clearly specified at present. See `proposal 84 `_ for an attempt. Since these rules are being removed, they are not part of the current proposal.) + +For example, the following are currently accepted:: + + data S = MkS { foo :: Int } + data T = MkT { foo :: Int, bar :: Int } + data U = MkU { bar :: Int, baz :: Int } + + d x = foo (x :: T) -- by rule 1 + + e = foo :: T -> Int -- by rule 2 + + f :: T -> Int + f = foo -- by rule 2 + + g = k foo -- by rule 2, assuming we already know k :: (T -> _) -> _ + +The following are currently rejected, and will remain so:: + + x = foo + + y = foo (MkT 42) -- argument does not have a type signature, so rule 1 does not apply + +Note that a type signature is absolutely required for rule 1 to apply; no inference is performed, even if it is "obvious" what the type of the argument is. + + +Proposed new rules for selectors +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Rules 1 and 2 are removed. There is no rule 3. Ambiguous field selector occurrences are rejected during name resolution. In particular, examples ``d``, ``e``, ``f`` and ``g`` will now be rejected. + + +Existing rules for updates +^^^^^^^^^^^^^^^^^^^^^^^^^^ +The rules for resolving ambiguous field update occurrences (i.e. those for which multiple fields with the same label are in scope) are currently as follows: + +4. If there is only one datatype that has all the fields being updated, use that datatype. + +5. If the expression being updated (i.e. the expression before the curly braces) has an explicit type signature determining a datatype, use that datatype. + +6. During bidirectional type-checking, if the type being pushed in to the record update determines a datatype, use that datatype. + +For example, the following are currently accepted by ``DuplicateRecordFields``:: + + data S = MkS { foo :: Int } + data T = MkT { foo :: Int, bar :: Int } + data U = MkU { bar :: Int, baz :: Int } + + d x = x { foo = 3, bar = 2 } -- by rule 4, only T has both fields + + e x = (x :: T) { foo = 3 } -- by rule 5 + + f x = x { foo = 3 } :: T -- by rule 6 + + g :: T -> T + g x = x { foo = 3 } -- by rule 6 + + h = k (x { foo = 3 }) -- by rule 6, assuming we already know k :: T -> _ + +The following are currently rejected, and will remain so:: + + let x :: T + x = blah + in x { foo = 3 } + + \x -> [x { foo = 3 }, blah :: T ] + + \ (x :: T) -> x { foo = 3 } + + +Proposed new rules for updates +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Rule 4 remains as above. Rules 5 and 6 are removed. Thus ambiguous field update occurrences are rejected during name resolution, except if there is a single datatype to which all the fields belong (which can be determined during name resolution rather than requiring information from type-checking). + +In particular, under the revised specification of ``DuplicateRecordFields``, example ``d`` will continue to be accepted, but examples ``e``, ``f``, ``g`` and ``h`` will now be rejected. + +Note that ``d`` is rejected when ``DuplicateRecordFields`` is disabled, because each field is required to be unambiguous in isolation, without considering the other fields in the update. + + +Transition period +^^^^^^^^^^^^^^^^^ +Since this proposal will break existing code using ``DuplicateRecordFields``, we propose a transition period involving the following steps: + +1. Introduce a new warning ``-Wambiguous-fields``, enabled by default. This will make the compiler emit a warning for every ambiguous field selector/update occurrence it resolves under the rules described above. The warning should explain that support for such occurrences will be removed in a future GHC release. + +2. In a subsequent GHC release, enable ``-Werror=ambiguous-fields``. This will mean that ambiguous field selector/update occurrences cause compilation to fail unless the user explicitly silences the warning. + +3. In a subsequent GHC release, remove support for ambiguous field selector/update occurrences entirely and remove the warning. This step should not be taken until ``RecordDotSyntax`` is available, to provide users with a clear alternative. + +This transition period will give time for users of ``DuplicateRecordFields`` to adapt their code (using ``RecordDotSyntax`` or otherwise), or raise concerns about the proposed changes and request a stay of execution. + + +Effect and Interactions +----------------------- +The new rules simplify the design and implementation of ``DuplicateRecordFields``, because the type-checker will no longer be involved in name resolution. Name information (including knowledge of which fields belong to which datatypes) will be sufficient to determine which field is referred to by every occurrence of a record selection or update. + +Under this proposal enabling ``DuplicateRecordFields`` for a module remains conservative, because any program that was accepted by the compiler without using the special selector disambiguation rules will still be accepted. However, existing programs already using ``DuplicateRecordFields`` may cease to be accepted. + +The ``RecordDotSyntax`` extension (`proposal 282 `_), and the ``HasField`` magic type class (`proposal 23 `_), provide alternative mechanisms for field selection and update. These do not apply in some rare circumstances (in particular, where fields have higher-rank or unboxed types), but in those cases users can use import hiding to limit the fields in scope and hence remove the ambiguity, or can write pattern-matching definitions instead of using record selectors. + +The ``NoFieldSelectors`` extension (`proposal 160 `_) changes datatypes so that they do not bring field selectors into scope at all. The current proposal complements ``NoFieldSelectors``, as it will make use of selectors under ``DuplicateRecordFields`` slightly less convenient. However, ``NoFieldSelectors`` affects definition sites, while the current proposal affects use sites, so until ``NoFieldSelectors`` is universally adopted, the current proposal is relevant for addressing the question of how ambiguous field selector occurrences should be resolved. + +The ``PatternSynonyms`` extension interacts awkwardly with the disambiguation rules in ``DuplicateRecordFields``, because record pattern synonyms may introduce new fields that work with existing types, so they do not work with type-directed name resolution. This proposal will make a proper integration of ``PatternSynonyms`` and ``DuplicateRecordFields`` easier, because this problem will be removed. + + +Costs and Drawbacks +------------------- +This change may be disappointing for users who would prefer more use of type information to resolve ambiguous names. Some users have already expressed this desire (e.g. see `issue #11343 `_). + +The change is backwards-incompatible for code that makes use of the ``DuplicateRecordFields`` extension. Accordingly we propose a transition period with a compatibility warning. + +The development cost of this change is relatively low (the new warning should be easy to implement, and the new specification mostly involves removing code). It should reduce maintenance costs of GHC overall. Moreover, since the specification of ``DuplicateRecordFields`` will be simpler, its behaviour will become easier to understand. + + +Alternatives +------------ +Keeping the status quo is entirely feasible, even though the current design is not completely satisfactory. This would allow us to wait until ``NoFieldSelectors`` and ``RecordDotSyntax`` have been tested in practice, before starting changes to ``DuplicateRecordFields``. + +We could take the opposite approach, and increase the use of type inference to resolve ambiguous selector occurrences, as requested by some users. However, it is not clear how to do this in anything other than an essentially ad hoc manner, so the extension is likely to become even more complex to specify and implement. + + +Unresolved Questions +-------------------- +Is the proposed transition period, with the new ``-Wambiguous-fields`` warning, appropriate? We could deprecate the feature even more gradually, or disable it without warning users first. + + +Implementation Plan +------------------- +If accepted, Adam Gundry will implement. The implementation does not depend on the implementation of any other proposals, although the proposed transition period will not end until ``RecordDotSyntax`` has been implemented and included in a GHC release. From cfc6056c2ed21539019555f94245d50d0e8f6f81 Mon Sep 17 00:00:00 2001 From: Adam Gundry Date: Sat, 26 Sep 2020 21:58:49 +0100 Subject: [PATCH 2/6] Re-title proposal as it relates to updates as well as selectors --- ...ous-selectors.rst => 0000-no-ambiguous-field-access.rst} | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename proposals/{0000-no-ambiguous-selectors.rst => 0000-no-ambiguous-field-access.rst} (97%) diff --git a/proposals/0000-no-ambiguous-selectors.rst b/proposals/0000-no-ambiguous-field-access.rst similarity index 97% rename from proposals/0000-no-ambiguous-selectors.rst rename to proposals/0000-no-ambiguous-field-access.rst index 0d779ae64d..ab3da8f3ce 100644 --- a/proposals/0000-no-ambiguous-selectors.rst +++ b/proposals/0000-no-ambiguous-field-access.rst @@ -1,5 +1,5 @@ -DuplicateRecordFields without ambiguous selectors -================================================= +DuplicateRecordFields without ambiguous field access +==================================================== .. author:: Adam Gundry .. date-accepted:: Leave blank. This will be filled in when the proposal is accepted. @@ -171,7 +171,7 @@ Alternatives ------------ Keeping the status quo is entirely feasible, even though the current design is not completely satisfactory. This would allow us to wait until ``NoFieldSelectors`` and ``RecordDotSyntax`` have been tested in practice, before starting changes to ``DuplicateRecordFields``. -We could take the opposite approach, and increase the use of type inference to resolve ambiguous selector occurrences, as requested by some users. However, it is not clear how to do this in anything other than an essentially ad hoc manner, so the extension is likely to become even more complex to specify and implement. +We could take the opposite approach, and increase the use of type inference to resolve ambiguous field occurrences, as requested by some users. However, it is not clear how to do this in anything other than an essentially ad hoc manner, so the extension is likely to become even more complex to specify and implement. Unresolved Questions From cc2bf5d63b004eacebc8678a36b873bd466d80ce Mon Sep 17 00:00:00 2001 From: Adam Gundry Date: Sat, 26 Sep 2020 22:04:20 +0100 Subject: [PATCH 3/6] Clarify that non-ambiguous occurrences are not affected --- proposals/0000-no-ambiguous-field-access.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/proposals/0000-no-ambiguous-field-access.rst b/proposals/0000-no-ambiguous-field-access.rst index ab3da8f3ce..824db28fd4 100644 --- a/proposals/0000-no-ambiguous-field-access.rst +++ b/proposals/0000-no-ambiguous-field-access.rst @@ -44,6 +44,8 @@ When ``DuplicateRecordFields`` is disabled, ambiguous field selector/update occu Note that if an identifier refers to both non-fields and fields, it is always rejected by the renamer as excessively ambiguous, regardless of ``DuplicateRecordFields``. +This proposal does not affect field selector/update occurrences that are not ambiguous. + Existing rules for selectors ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From 8e24a790ba279dbc0e68a91a795ba9afb382261d Mon Sep 17 00:00:00 2001 From: Adam Gundry Date: Wed, 30 Sep 2020 13:42:08 +0100 Subject: [PATCH 4/6] Amend transition period to take into account feedback --- proposals/0000-no-ambiguous-field-access.rst | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/proposals/0000-no-ambiguous-field-access.rst b/proposals/0000-no-ambiguous-field-access.rst index 824db28fd4..e1520f6e42 100644 --- a/proposals/0000-no-ambiguous-field-access.rst +++ b/proposals/0000-no-ambiguous-field-access.rst @@ -140,11 +140,11 @@ Since this proposal will break existing code using ``DuplicateRecordFields``, we 1. Introduce a new warning ``-Wambiguous-fields``, enabled by default. This will make the compiler emit a warning for every ambiguous field selector/update occurrence it resolves under the rules described above. The warning should explain that support for such occurrences will be removed in a future GHC release. -2. In a subsequent GHC release, enable ``-Werror=ambiguous-fields``. This will mean that ambiguous field selector/update occurrences cause compilation to fail unless the user explicitly silences the warning. +2. In a subsequent GHC release, remove support for ambiguous field selector/update occurrences entirely and remove the warning. This step should not be taken until ``RecordDotSyntax`` is available, to provide users with a clear alternative. -3. In a subsequent GHC release, remove support for ambiguous field selector/update occurrences entirely and remove the warning. This step should not be taken until ``RecordDotSyntax`` is available, to provide users with a clear alternative. +This transition period will give time for users of ``DuplicateRecordFields`` to adapt their code (using ``RecordDotSyntax`` or otherwise), or raise concerns about the proposed changes and request a stay of execution. Our expectation is that step 2 will be taken in the GHC release immediately following step 1, but this can be changed if feedback from users indicates that the removal of the feature is causing substantial pain. -This transition period will give time for users of ``DuplicateRecordFields`` to adapt their code (using ``RecordDotSyntax`` or otherwise), or raise concerns about the proposed changes and request a stay of execution. +The warning produced by ``-Wambiguous-fields`` should mention the specific selector and type that were determined by the disambiguation rules, rather than just complaining about the ambiguity. This should make it easier for affected users to adapt their code. Effect and Interactions @@ -175,10 +175,12 @@ Keeping the status quo is entirely feasible, even though the current design is n We could take the opposite approach, and increase the use of type inference to resolve ambiguous field occurrences, as requested by some users. However, it is not clear how to do this in anything other than an essentially ad hoc manner, so the extension is likely to become even more complex to specify and implement. +We could extend or shorten the transition period. The current proposal strikes a balance between the desire to not break users' code without warning, and the desire to simplify the implementation. + Unresolved Questions -------------------- -Is the proposed transition period, with the new ``-Wambiguous-fields`` warning, appropriate? We could deprecate the feature even more gradually, or disable it without warning users first. +None. Implementation Plan From 46ea62241f64c224e6b7c81f5ac5ac6fb422c95a Mon Sep 17 00:00:00 2001 From: Adam Gundry Date: Tue, 27 Oct 2020 16:05:04 +0000 Subject: [PATCH 5/6] Document migration strategy --- proposals/0000-no-ambiguous-field-access.rst | 57 +++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/proposals/0000-no-ambiguous-field-access.rst b/proposals/0000-no-ambiguous-field-access.rst index e1520f6e42..80548abe7b 100644 --- a/proposals/0000-no-ambiguous-field-access.rst +++ b/proposals/0000-no-ambiguous-field-access.rst @@ -23,7 +23,7 @@ The ``DuplicateRecordFields`` extension is useful to allow multiple datatypes to module M where data Person = MkPerson { name :: String, pets :: [Pet] } - data Pet = MkPet { name :: Text } + data Pet = MkPet { name :: Int } It is now entirely unproblematic to use ``name`` in a record construction or pattern match, because the constructor name disambiguates which field is meant:: @@ -147,6 +147,61 @@ This transition period will give time for users of ``DuplicateRecordFields`` to The warning produced by ``-Wambiguous-fields`` should mention the specific selector and type that were determined by the disambiguation rules, rather than just complaining about the ambiguity. This should make it easier for affected users to adapt their code. +Migration strategy +^^^^^^^^^^^^^^^^^^ +Code that is broken by this proposal because it relies on ambiguous field occurrences can be fixed in one of the following ways: + +1. Where the field is defined in a different module, use qualified imports, import hiding and/or aliases to remove the ambiguity. For example, here is a `technique using import aliases `_:: + + {-# LANGUAGE DuplicateRecordFields #-} + module M1 where + data Person = MkPerson { name :: String, pets :: [Pet] } + data Pet = MkPet { name :: Int } + + module N where + import M1 as Person (Person(..)) + import M1 as Pet (Pet(..)) + + getPersonName :: Person -> String + getPersonName = Person.name + + setPersonName :: String -> Person -> Person + setPersonName n p = p { Person.name = n } + + The new version of the code is completely backwards-compatible, and its meaning is clear. The downsides are that this approach cannot be used within a single module, barring enhancements to the module system, and it requires boilerplate imports. + +2. Use ``RecordDotSyntax`` when it is available:: + + {-# LANGUAGE DuplicateRecordFields, RecordDotSyntax #-} + module M2 where + data Person = MkPerson { name :: String, pets :: [Pet] } + data Pet = MkPet { name :: Int } + + getPersonName :: Person -> String + getPersonName p = p.name + + setPersonName :: String -> Person -> Person + setPersonName n p = p { name = n } + + This works within a single module. However it requires a new as-yet-unreleased extension, and will not work for fields with higher-rank or unboxed types. + +3. Use explicit pattern-matching and record construction, possibly in combination with ``NamedFieldPuns`` or ``RecordWildCards``:: + + {-# LANGUAGE DuplicateRecordFields, NamedFieldPuns #-} + module M2 where + data Person = MkPerson { name :: String, pets :: [Pet] } + data Pet = MkPet { name :: Int } + + getPersonName :: Person -> String + getPersonName MkPerson{name} = name + + setPersonName :: String -> Person -> Person + setPersonName n MkPerson{pets} = MkPerson {name = n, pets } + + This works in a single module and does not require any new extensions, but it may require additional boilerplate, especially if a type has many constructors and/or fields. + + + Effect and Interactions ----------------------- The new rules simplify the design and implementation of ``DuplicateRecordFields``, because the type-checker will no longer be involved in name resolution. Name information (including knowledge of which fields belong to which datatypes) will be sufficient to determine which field is referred to by every occurrence of a record selection or update. From ed193a1d8d762b6572bb38706a9cdb654784e16f Mon Sep 17 00:00:00 2001 From: Adam Gundry Date: Tue, 27 Oct 2020 16:06:21 +0000 Subject: [PATCH 6/6] Remove hard dependency on RecordDotSyntax, other minor edits --- proposals/0000-no-ambiguous-field-access.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/proposals/0000-no-ambiguous-field-access.rst b/proposals/0000-no-ambiguous-field-access.rst index 80548abe7b..a5048f60f5 100644 --- a/proposals/0000-no-ambiguous-field-access.rst +++ b/proposals/0000-no-ambiguous-field-access.rst @@ -140,7 +140,7 @@ Since this proposal will break existing code using ``DuplicateRecordFields``, we 1. Introduce a new warning ``-Wambiguous-fields``, enabled by default. This will make the compiler emit a warning for every ambiguous field selector/update occurrence it resolves under the rules described above. The warning should explain that support for such occurrences will be removed in a future GHC release. -2. In a subsequent GHC release, remove support for ambiguous field selector/update occurrences entirely and remove the warning. This step should not be taken until ``RecordDotSyntax`` is available, to provide users with a clear alternative. +2. In a subsequent GHC release, remove support for ambiguous field selector/update occurrences entirely and remove the warning. This step should not be taken until ``RecordDotSyntax`` or another generally-accepted mechanism for disambiguation is available, to provide users with a clear alternative. This transition period will give time for users of ``DuplicateRecordFields`` to adapt their code (using ``RecordDotSyntax`` or otherwise), or raise concerns about the proposed changes and request a stay of execution. Our expectation is that step 2 will be taken in the GHC release immediately following step 1, but this can be changed if feedback from users indicates that the removal of the feature is causing substantial pain. @@ -219,7 +219,7 @@ Costs and Drawbacks ------------------- This change may be disappointing for users who would prefer more use of type information to resolve ambiguous names. Some users have already expressed this desire (e.g. see `issue #11343 `_). -The change is backwards-incompatible for code that makes use of the ``DuplicateRecordFields`` extension. Accordingly we propose a transition period with a compatibility warning. +The change is backwards-incompatible for code that makes use of the ``DuplicateRecordFields`` extension. Accordingly we propose a transition period with a compatibility warning. Even so, removing the feature may `cause user dissatisfaction `_. The development cost of this change is relatively low (the new warning should be easy to implement, and the new specification mostly involves removing code). It should reduce maintenance costs of GHC overall. Moreover, since the specification of ``DuplicateRecordFields`` will be simpler, its behaviour will become easier to understand. @@ -240,4 +240,4 @@ None. Implementation Plan ------------------- -If accepted, Adam Gundry will implement. The implementation does not depend on the implementation of any other proposals, although the proposed transition period will not end until ``RecordDotSyntax`` has been implemented and included in a GHC release. +If accepted, Adam Gundry will implement. A `draft implementation of the warning `_ exists already. The implementation of the warning does not depend on the implementation of any other proposals, although the proposed transition period depends on the implementation of ``RecordDotSyntax``.