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

Use -XNoStrictData in generated parser (#273) #289

Merged
merged 2 commits into from
Sep 13, 2024
Merged

Use -XNoStrictData in generated parser (#273) #289

merged 2 commits into from
Sep 13, 2024

Conversation

sgraf812
Copy link
Collaborator

Fixes #273. CC @andreasabel

@@ -4,6 +4,9 @@

The main focus of this release was modularizing Happy.

* Generated parsers now activate the language extension `-XNoStrictData` without
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-X(No)StrictData is available since GHC-8.0, so this change means that happy won't anymore generate code which works with older GHCs.

As far as I'm concerned, that is fine, but it's probably a good idea to tell explicitly in the changelog.

Copy link
Contributor

@phadej phadej Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... you don't test master with older than GHC-8.0, though it might be still buildable with older GHCs? If it weren't, then using new happy with old GHCs will become very unlikely.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trouble is that it has become quite hard to test 7.10 and below in CI. ghcup doesn't provide it and I'm not a huge fan of manually setting it up. Besides it is not unlikely that it won't work due to some glibc incompatibility.

Besides, GHC 8.0 is quite ancient at this point and I would be happy to drop support for 7.10 and below. The cabal file says as much already, but perhaps we should announce it in the release notes as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cabal file says as much already,

It doesn't

It only says that you test with GHC-8.0 and up, not that you don't support oldere GHCs. E.g. you have

  build-depends: base < 5,
                 array,
                 ...

so technically cabal would try compile even with GHC-7.0 etc.

And it actually fails, if you test it:

Building library for happy-backend-glr-2.0..
ghc: unrecognised flag: -Wincomplete-uni-patterns

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyone stuck on GHC < 8.0 is probably also fine with happy < 2.0. Let's not waste time on legacy compiler versions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@int-index Correct the metadata. Write lower-bounds. base >=4.9. Not a hard thing to do. It's not a waste of time.

Copy link
Collaborator

@int-index int-index Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependency bounds for the generator itself and dependency bounds for the generated code aren't the same thing. base >=4.9 is correct as long as happy itself builds with base >=4.9.

I agree that we should bump this bound because, as you point out, happy-backend-glr doesn't build with older GHCs. However it's a separate problem unrelated to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, if happy itself is not buildable with older GHCs then it's make sense it isn't able to generate code for older GHCs.

It could be so that happy is buildable only with newer GHCs (say even GHC-9+ only) but supports code generation for older GHCs. (It would be trickier to test but not impossible. E.g. cabal is buildable with only newer GHCs, but can work with older than that).

It would be somewhat confusing if happy would be buildable with old GHCs, but not be able to generate code for those old GHCs. Given that cabal-install builds happy itself (doesn't reuse some global installation), those users would find themselves if complicated situations.

So, IMHO, it makes everything somewhat simpler if happy itself and generated code has the same support GHC windows. "If you can build a happy version with GHC-x.y, that happy will generate code which GHC-x.y will handle".

In particular, if/when GHC introduces a change, such that happy itself is still buildable, but code is not, I'd argue that happy shouldn't be buildable.

There is no way to declare in happy.cabal dependency metadata for the generated code, so at large it's simpler if happy's own build metadata is (re)used also as the generated code dependency metadata. (Similarly as we are currently imprecise in multi-staged or cross-compilation scenarios, using the same bounds for all stages and host&target).

Copy link
Collaborator

@int-index int-index left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had to insert {-# LANGUAGE NoStrictData #-} manually on multiple occasions, so I support fixing this in the code generator.

@sgraf812
Copy link
Collaborator Author

sgraf812 commented Sep 13, 2024

Alright, I added another commit setting a lower bound on base >= 4.9, simply because we discussed the issue here. I will merge soon. We can always revert if someone feels overlooked.

@sgraf812 sgraf812 merged commit 48d3480 into master Sep 13, 2024
26 checks passed
@sgraf812 sgraf812 deleted the wip/T273 branch September 13, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Happy-generated code fails with Internal Happy error with -XStrictData
3 participants