Fixes an issue with deprecated symbols that are explicitly imported not throwing a warning
Needs RevisionPublic

Authored by alanasp on Jul 3 2018, 2:36 PM.

Details

Reviewers
bgamari
tdammers
Trac Issues
#15337
Summary

A relevant test (rn070) is also added to the testsuite

The fix caused an issue with libraries/bytestring reexporting a deprecated symbol internally. I have added a pragma {-# OPTIONS_GHC -fno-warn-warnings-deprecations #-} to prevent this from happening.

Signed-off-by: Alanas Plascinskas <alanas.pla@gmail.com>

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
fix-trac-15337
Lint
Lint WarningsExcuse: as before, other lines in this file are of similar length
SeverityLocationCodeMessage
Warningcompiler/rename/RnNames.hs:51TXT3Line Too Long
Warningcompiler/rename/RnNames.hs:367TXT3Line Too Long
Warningcompiler/rename/RnNames.hs:370TXT3Line Too Long
Warningcompiler/rename/RnNames.hs:384TXT3Line Too Long
Warningcompiler/rename/RnNames.hs:386TXT3Line Too Long
Warningcompiler/rename/RnNames.hs:387TXT3Line Too Long
Warningcompiler/rename/RnNames.hs:402TXT3Line Too Long
Warningcompiler/rename/RnNames.hs:404TXT3Line Too Long
Warningcompiler/rename/RnNames.hs:405TXT3Line Too Long
Unit
No Unit Test Coverage
Build Status
Buildable 21632
Build 49344: [GHC] Linux/amd64: Continuous Integration
Build 49343: [GHC] OSX/amd64: Continuous Integration
Build 49342: [GHC] Windows/amd64: Continuous Integration
Build 49341: arc lint + arc unit
alanasp created this revision.Jul 3 2018, 2:36 PM
alanasp updated this revision to Diff 17175.Jul 3 2018, 2:59 PM
  • compiler: fix trac issue 15337
  1. Updating D4931: compiler: add tests to issue 15337 #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

Deprecated symbols imported explicitly will now throw a warning

alanasp retitled this revision from compiler: add tests to issue 15337 to compiler: fix trac issue 15337.Jul 3 2018, 3:02 PM
alanasp edited the summary of this revision. (Show Details)
alanasp updated the Trac tickets for this revision.
tdammers requested changes to this revision.Jul 4 2018, 10:43 AM
tdammers added a subscriber: tdammers.
  • Please fix the parser error / build failure
  • A more descriptive commit message would be great
compiler/rename/RnNames.hs
508

This function and the subsequent ones seem to be accidentally indented by one space, which causes parser errors (see https://phabricator.haskell.org/harbormaster/build/49233/).

This revision now requires changes to proceed.Jul 4 2018, 10:43 AM
alanasp updated this revision to Diff 17197.Jul 5 2018, 7:51 AM
  • compiler: fix trac issue 15337
alanasp retitled this revision from compiler: fix trac issue 15337 to The change makes deprecated symbols that are explicitly imported throw a warning.Jul 5 2018, 8:24 AM
alanasp edited the summary of this revision. (Show Details)
alanasp added subscribers: mpickering, erikd.
alanasp retitled this revision from The change makes deprecated symbols that are explicitly imported throw a warning to Fixes an issue with deprecated symbols that are explicitly imported not throwing a warning.Jul 5 2018, 8:26 AM
alanasp edited the summary of this revision. (Show Details)

I am not sure why this is failing a Linux build, it works fine on my machine

Well, here's the relevant bit from the build log:

libraries/bytestring/Data/ByteString/Char8.hs:243:1: error: [-Wdeprecations, -Werror=deprecations]
    Deprecated: "findSubstring is deprecated in favour of breakSubstring."
    |
243 | import Data.ByteString (empty,null,length,tail,init,append
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...

libraries/bytestring/Data/ByteString/Char8.hs:243:1: error: [-Wdeprecations, -Werror=deprecations]
    Deprecated: "findSubstrings is deprecated in favour of breakSubstring."
    |
243 | import Data.ByteString (empty,null,length,tail,init,append
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...
make[1]: *** [libraries/bytestring/dist-install/build/Data/ByteString/Char8.o] Error 1

This is kind of funny, because it is exactly what this patch is trying to achieve - except that GHC itself (or rather, the Data.ByteString.Char8 module from the bytestring boot library) imports deprecated symbols itself, so when you build the stage2 compiler, the stage1 compiler, which already includes this patch, raises a warning, and because we have that particular warning configured as an error (-Werror=deprecations), the build fails.

So this isn't a mistake on your part, on the contrary - your patch reveals an existing (though minor) bug.

The proper way to fix this would be to change the Char8 module to use breakSubstring instead of findSubstrings; however this seems a bit out-of-scope for this patch. OTOH, as it is, the code won't build cleanly, so I'm unsure how we should proceed.

I've looked a bit deeper, and it turns out that the "bug" isn't really a bug - essentially what bytestring is doing here is re-exporting findSubstring from Data.ByteString from Data.ByteString.Char8. More details in the ticket (Trac #15337).

alanasp updated this revision to Diff 17261.Jul 10 2018, 12:52 PM
  • changed .gitmodules libraries/bytestring url
  • libraries/bytestring updated
alanasp updated this revision to Diff 17264.Jul 10 2018, 3:31 PM
  • warning fixes

I have forked the bytestring library and added a pragma {-# OPTIONS_GHC -fno-warn-warnings-deprecations #-} to Char8.hs file, so it wouldn't complain. It seems that now, after a few more fixes, it is finally able to build.

alanasp edited the summary of this revision. (Show Details)Jul 11 2018, 3:30 AM
tdammers accepted this revision.Sep 27 2018, 9:14 AM
This revision is now accepted and ready to land.Sep 27 2018, 9:14 AM
bgamari requested changes to this revision.Jan 20 2019, 7:41 PM

@alansp, I just found this sitting in the merge queue. Unfortunately it doesn't appear to apply and I'm having some trouble resolving the conflicts. Do you suppose you could have a look? My apologies for dropping the ball on this one.

This revision now requires changes to proceed.Jan 20 2019, 7:41 PM

@bgamari I think this revision is now irrelevant as it is all part of D4953