Opened 3 years ago

Last modified 3 weeks ago

#12389 new feature request

Limit duplicate export warnings for datatypes

Reported by: dfeuer Owned by: parsonsmatt
Priority: normal Milestone: 8.10.1
Component: Compiler Version: 7.10.3
Keywords: newcomer Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect warning at compile-time Test Case:
Blocked By: Blocking:
Related Tickets: #11959 Differential Rev(s): Phab:D4134
Wiki Page:

Description

containers sometimes exports data constructors/patterns conditionally (depending on GHC version and whether TESTING is defined). Presently, to avoid a duplicate export warning, it's necessary to write a separate export line for each combination of exported constructors, which is horrible:

module Foo (
#ifdef TESTING
#ifdef USE_PATTERN_SYNONYMS
  Foo (Foo, Pat1, Pat2)
#else
  Foo (Foo)
#endif
#elif USE_PATTERN_SYNONYMS
  Foo (Pat1, Pat2)
#else
  Foo
#endif

or to break up the lines with CPP, which is so horrible I can't even bring myself to write it.

I'd much rather be able to write

module Foo (
   Foo
#ifdef TESTING
  ,Foo(Foo)
#endif
#ifdef USE_PATTERN_SYNONYMS
  ,Foo(Pat1, Pat2)
#endif
  )

The trouble here is that GHC warns about duplicate export of the type Foo. I think there's a pretty simple partial solution: only warn about a type export that is *completely* redundant, adding neither type constructor nor pattern. And offer a way to turn off the redundant export warning entirely for types and classes without turning it off for bindings.

Change History (18)

comment:1 Changed 3 years ago by rwbarton

Well the first Foo export certainly looks redundant (as long as at least one of the CPP flags is enabled).

I don't really see how your second version is any better than what you say is "so horrible [...]":

module Foo (
   Foo (
#ifdef TESTING
     Foo,
#endif
#ifdef USE_PATTERN_SYNONYMS
     Pat1, Pat2,
#endif
   )
  )

(A trailing comma is legal here right? Otherwise it really is a mess.)

comment:2 in reply to:  1 Changed 3 years ago by dfeuer

Replying to rwbarton:

Well the first Foo export certainly looks redundant (as long as at least one of the CPP flags is enabled).

I don't really see how your second version is any better than what you say is "so horrible [...]":

module Foo (
   Foo (
#ifdef TESTING
     Foo,
#endif
#ifdef USE_PATTERN_SYNONYMS
     Pat1, Pat2,
#endif
   )
  )

(A trailing comma is legal here right? Otherwise it really is a mess.)

I don't know about GHC 8, but in 7.10, a trailing comma is *not* legal, and neither is a leading one.

comment:3 Changed 3 years ago by rwbarton

Hmm, it's not (in 8 either). A trailing comma is legal at the end of the export list though. Seems like we should just allow it at the end of a constructor/etc. list also.

comment:4 Changed 3 years ago by dfeuer

Sure. That would solve (much of) the problem. I'd want leading commas too, because I tend to prefer a leading-comma style. And I'd want to silence the warning from duplicate constructors/patterns within the list. Because I could, hypothetically, have

  Foo (
#if Condition1
      Foo, Bar
#endif
#if Condition2
    , Bar, Baz
#endif
    )

and wouldn't want to have to get fancy with the CPP if Condition1 and Condition2 both happen to hold.

comment:5 Changed 3 years ago by rwbarton

Well you don't really want to "silence the warning from duplicate constructors/patterns within the list". You want to silence this duplicate of Bar. If you happen to repeat a different constructor elsewhere in the export list by accident, you'd still want a warning about that. So it would more appropriate to use a mechanism for locally disabling warnings.

Since you can already write what you need with #if Condition1 || Condition2, it's not a problem anyways. At some point you may be better off just disabling the warning entirely. Warnings about redundancies are too hard when the compiler can't see your entire program at once.

comment:6 in reply to:  5 Changed 3 years ago by dfeuer

Replying to rwbarton:

Well you don't really want to "silence the warning from duplicate constructors/patterns within the list". You want to silence this duplicate of Bar. If you happen to repeat a different constructor elsewhere in the export list by accident, you'd still want a warning about that. So it would more appropriate to use a mechanism for locally disabling warnings.

Local suppression is very heavy for a warning about something as relatively unimportant as this. I'd be happy with a heuristic: accidental duplicates are typically less likely in a constructor/pattern export list than in a (typically much longer) module export list.

comment:7 Changed 3 years ago by rwbarton

Well I wouldn't be too happy with that; the flag should do what it says on the tin and cater to the needs of normal users over those of CPP-wielding maniacs :)

comment:8 Changed 3 years ago by dfeuer

Fine; if I can get leading and trailing commas, I'll be happy.

comment:9 Changed 3 years ago by simonpj

If I remember right, the export list itself does allow extra commas. So it seems a bit inconsistent not to allow the same story for the Foo( P1, P2 ) form.

So this would be ok with me.

comment:10 Changed 2 years ago by rwbarton

Milestone: 8.2.18.4.1

comment:11 Changed 2 years ago by rwbarton

Keywords: newcomer added

comment:12 Changed 15 months ago by parsonsmatt

I'm going to attempt a patch for this. By my first look, it should require a change to the compiler/parser/Parser.y Happy file, specifically the qcnames parse rule.

comment:13 Changed 15 months ago by parsonsmatt

Differential Rev(s): Phab:D4134
Owner: set to parsonsmatt

comment:14 Changed 15 months ago by simonpj

I'm going to attempt a patch for this.

Thanks! But what exactly is "this"?

I think you mean something like "allow leading and trailing commas in the sub-export lists". As I said above, that's ok with me. but

  • It should be consistent with exports lists themselves. Do they allow leading commas? If not, it'd make sense to add them. Thus
    module M( , f, g, ) where ...
    
  • Do we allow multiple leading or trailing commas? What about repeated commas in the middle of a list?
  • What about import lists? Should they not be consistent?
  • Should we require a language extension flag?

So although this is a small and superficial change, it is a user-facing one, and so should really go through the GHC proposals process. Because it's small, it'll be quick! But the debate is always helpful.

I'm conscious that this may seem obstructive, but I've learned that it really is worth writing down the specification and debating it before implementing it. Doing so materially increases quality by giving a way for more people to contribute.

Thanks for helping!

Simon

comment:15 in reply to:  description Changed 15 months ago by parsonsmatt

You're correct -- that is what I meant :)

I've collected the notes into a GHC Proposal #87. I'll submit the proposal to the wider community and see what comes of that.

Thanks!

comment:16 Changed 12 months ago by bgamari

Milestone: 8.4.18.6.1

This ticket won't be resolved in 8.4; remilestoning for 8.6. Do holler if you are affected by this or would otherwise like to work on it.

comment:17 Changed 10 months ago by bgamari

Milestone: 8.6.18.8.1

Thanks for taking this on, parsonsmatt! However, given that the Proposal is going to be revised I doubt this will happen for 8.6. Bumping.

comment:18 Changed 3 weeks ago by osa1

Milestone: 8.8.18.10.1

Bumping milestones of low-priority tickets.

Note: See TracTickets for help on using tickets.