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

Refactor to avoid incomplete record selection warning #10402

Closed
simonpj opened this issue Oct 1, 2024 · 4 comments · Fixed by #10403
Closed

Refactor to avoid incomplete record selection warning #10402

simonpj opened this issue Oct 1, 2024 · 4 comments · Fixed by #10403
Assignees
Labels
re: code quality Internal issue: concerning code quality / refactorings type: refactor

Comments

@simonpj
Copy link
Collaborator

simonpj commented Oct 1, 2024

When I compile cabal with -Wincomplete-record-selectors I get this:

libraries/Cabal/Cabal/src/Distribution/Simple/GHC/Build/Link.hs:637:115: error: [GHC-17335] [-Wincomplete-record-selectors, Werror=incomplete-record-selectors]
    Selecting the record field ‘componentExposedModules’ may fail for the following constructors:
      FLibComponentLocalBuildInfo, ExeComponentLocalBuildInfo, TestComponentLocalBuildInfo, BenchComponentLocalBuildInfo
    |
637 |               reexported_modules = [mn | LibComponentLocalBuildInfo{} <- [clbi], IPI.ExposedModule mn (Just{}) <- componentExposedModules clbi]
    |                                                                                                                   ^^^^^^^^^^^^^^^^^^^^^^

Now, it's true the the record selector componentExposedModules cannot fail when applied to clbi, but the reasoning for that is very indirect: it means understanding that LibComponentLocalBuildInfo{} <- [clbi] guarantees that clbi is definitely built with LibComponentLocalBuildInfo, which itself involves knowlege of how list comprehensions work, and singleton lists. This far beyond what the incomplete-record-selector warning mechanisms can do.

Suggestion: the code would be more perspicuous and robust if we said

         reexported_modules = [mn | LibComponentLocalBuildInfo{ componentExposedModules = exposed_mods } <- [clbi]
                                  , IPI.ExposedModule mn (Just{}) <- exposed_mods]

Now no warning! And it's a fully backward-compatible change.

How did this arise?

It turns out that GHC proposal 516 Incomplete record selectors said that -Wincomplete-record-selectors should be in -Wall, but due to an error it was omitted. When I add it to -Wall, to make GHC follow the proposal, GHC's build system falls over when compiling Cabal because it uses -Werror.

For now, we've agreed that I'll add -Wno-incomplete-record-selectors to GHC's build system for Cabal. But if the Cabal authors adopt the above change, we can stop suppressing those warnnings. Thank you!

@geekosaur
Copy link
Collaborator

geekosaur commented Oct 1, 2024

What version is libraries/Cabal pinned to? That's line 738 on master and probably 3.14. (I'm preparing a PR on master that should backport readily to 3.14, but I think libraries/Cabal is on something older that's missing e.g. the tagged files refactor.)

geekosaur added a commit to geekosaur/cabal that referenced this issue Oct 1, 2024
geekosaur added a commit to geekosaur/cabal that referenced this issue Oct 1, 2024
hubot pushed a commit to ghc/ghc that referenced this issue Oct 1, 2024
This change is a temporary fix, until
   haskell/cabal#10402
is addressed
@simonpj
Copy link
Collaborator Author

simonpj commented Oct 1, 2024

Thanks! I don't know about branches... maybe @mpickering can help.

@geekosaur geekosaur added type: refactor re: code quality Internal issue: concerning code quality / refactorings and removed needs triage type: bug labels Oct 1, 2024
hubot pushed a commit to ghc/ghc that referenced this issue Oct 2, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402
@mpickering
Copy link
Collaborator

My understanding according to the release timelines on #10193 is that there will be 3.16 cut from the master branch before the 9.12 release so no backports to 3.14 branch will be needed.

@mpickering
Copy link
Collaborator

It seems that in #10193 the consensus is now there will not be a 3.16 release before ghc-9.12. Sorry for that misunderstanding, I was going by what was written in the ticket.

However, the patch in question which requires this change will not be in 9.12, so there is no need to hurry a backport.

Mikolaj pushed a commit to geekosaur/cabal that referenced this issue Oct 3, 2024
@mergify mergify bot closed this as completed in #10403 Oct 3, 2024
hubot pushed a commit to ghc/ghc that referenced this issue Oct 4, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402

2.6% decrease in compile time allocation in RecordUpPerf

Metric Decrease:
    RecordUpdPerf
hubot pushed a commit to ghc/ghc that referenced this issue Oct 5, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402

2.6% decrease in compile time allocation in RecordUpPerf

Metric Decrease:
    RecordUpdPerf
hubot pushed a commit to ghc/ghc that referenced this issue Oct 7, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402

2.6% decrease in compile time allocation in RecordUpPerf

Metric Decrease:
    RecordUpdPerf
hubot pushed a commit to ghc/ghc that referenced this issue Oct 8, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402

2.6% decrease in compile time allocation in RecordUpPerf

Metric Decrease:
    RecordUpdPerf
hubot pushed a commit to ghc/ghc that referenced this issue Oct 9, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402

2.6% decrease in compile time allocation in RecordUpPerf

Metric Decrease:
    RecordUpdPerf
hubot pushed a commit to ghc/ghc that referenced this issue Oct 9, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402

2.6% decrease in compile time allocation in RecordUpPerf

Metric Decrease:
    RecordUpdPerf
hubot pushed a commit to ghc/ghc that referenced this issue Oct 10, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402

2.6% decrease in compile time allocation in RecordUpPerf

Metric Decrease:
    RecordUpdPerf
hubot pushed a commit to ghc/ghc that referenced this issue Oct 10, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402

2.6% decrease in compile time allocation in RecordUpPerf

Metric Decrease:
    RecordUpdPerf
hubot pushed a commit to ghc/ghc that referenced this issue Oct 11, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402

2.6% decrease in compile time allocation in RecordUpPerf

Metric Decrease:
    RecordUpdPerf
mergify bot pushed a commit that referenced this issue Oct 12, 2024
Per SPJ suggection.

Closes: #10402
(cherry picked from commit b1798b5)
mergify bot pushed a commit that referenced this issue Oct 12, 2024
Per SPJ suggection.

Closes: #10402
(cherry picked from commit b1798b5)
hubot pushed a commit to ghc/ghc that referenced this issue Oct 13, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402

2.6% decrease in compile time allocation in RecordUpPerf

Metric Decrease:
    RecordUpdPerf
hubot pushed a commit to ghc/ghc that referenced this issue Oct 14, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402

2.6% decrease in compile time allocation in RecordUpPerf

Metric Decrease:
    RecordUpdPerf
hubot pushed a commit to ghc/ghc that referenced this issue Oct 15, 2024
This commit does several related things:

* Major refactor of the handling of applications in the desugarer.
  Now all applications are handled in `dsApp`, `ds_app` and related
  functions.  This dramatically simplifies the code and removes
  complicated cruft that had accumulated.  Hooray.

  Fixes #25281.

* Improve the handling of -Wincomplete-record-selectors.

  We now incorporate the result type of unsaturated record selector
  applications as well as consider long-distance information in
  getField applications.

  Plus, the implmentation now builds the improved `dsApp` stuff
  above, so it is much easier to understand.

  Plus, incorporates improved error message wording suggested
  by Adam Gundry in !12685.

  Fixes #24824, #24891

  See the long Note [Detecting incomplete record selectors]

* Add -Wincomplete-record-selectors to -Wall, as specified in
  GHC Proposal 516.

  To do this, I also had to add -Wno-incomplete-record-selectors
  to the build flags for Cabal in GHC's CI.  See
  hadrian/src/Settings/Warnings.hs.  We can remove this when
  Cabal is updated so that it doesn't trigger the warning:
  haskell/cabal#10402

2.6% decrease in compile time allocation in RecordUpPerf

Metric Decrease:
    RecordUpdPerf
geekosaur added a commit that referenced this issue Oct 29, 2024
Per SPJ suggection.

Closes: #10402
(cherry picked from commit b1798b5)
Mikolaj pushed a commit that referenced this issue Nov 7, 2024
Per SPJ suggection.

Closes: #10402
(cherry picked from commit b1798b5)
erikd pushed a commit to erikd/cabal that referenced this issue Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
re: code quality Internal issue: concerning code quality / refactorings type: refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants