Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#9988 closed task (fixed)

Remove fun_id, is_infix from FunBind, as they are now in Match

Reported by: alanz Owned by: alanz
Priority: highest Milestone: 7.10.1
Component: Compiler Version: 7.10.1-rc1
Keywords: Cc: simonpj
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D639
Wiki Page:

Description (last modified by alanz)

One of the changes Phab:D538 introduced is to add m_fun_id_infix to Match

data Match id body
  = Match {
        m_fun_id_infix :: (Maybe (Located id,Bool)),
          -- fun_id and fun_infix for functions with multiple equations
          -- only present for a RdrName. See note [fun_id in Match]
        m_pats :: [LPat id], -- The patterns
        m_type :: (Maybe (LHsType id)),
                                 -- A type signature for the result of the match
                                 -- Nothing after typechecking
        m_grhss :: (GRHSs id body)
  } deriving (Typeable)

This was done to track the individual locations and fixity of the fun_id for each of the defining equations for a function when there are more than one.

For example, the function (&&&) is defined with some prefix and some infix equations below.

(&&&  ) [] [] =  []
xs    &&&   [] =  xs
(  &&&  ) [] ys =  ys

This means that the fun_id / is_infix is now superfluous in the FunBind. This has not been removed as a potentially risky change just before 7.10 RC2, and so must be done after.

This ticket captures that task, which includes processing these fields through the renamer and beyond.

Change History (16)

comment:1 Changed 5 years ago by alanz

Owner: set to alanz

comment:2 Changed 5 years ago by alanz

Description: modified (diff)

comment:3 Changed 5 years ago by simonpj

Actually I'm not sure it'd be a good idea to remove it. It's quite convenient to have the function name in the FunBind.

But I think we can (and should) remove fun_infix from FunBind.

Simon

comment:4 Changed 5 years ago by hvr

Milestone: 7.12.1

I assume this one's targetted for GHC 7.12.1 (rather than GHC 7.10.2?)

comment:5 Changed 5 years ago by alanz

Yes, but I have just realised I may need this functionality, ghc-exactprint may be able to generate output from RenamedSource, which makes HaRe integration easier.

If I got it done soon could it hit 7.10.2? I am currently putting a workaround into ghc-exactprint, so it is not critical, but would be cleaner.

comment:6 Changed 5 years ago by simonpj

7.10 is in RC2. We can't keep modifying it otherwise we will never get it out. I suppose that if you have strong reason to believe that you are not introducing new bugs, and having it in is going to transform your life for the better, then you can make the case.

comment:7 Changed 5 years ago by alanz

Phab:D639 refers, which does not close this ticket, but is just the first step toward it.

I strongly believe I am not introducing new bugs as it is a simple addition to rnMatch' with pure code that only sets a variable that is ignored in the rest of the compilation process, being introduced specifically for API Annotations.

In terms of motivation, the current HaRe model is to do analysis on the RenamedSource, then transform the ParsedSource. Having funcction id locations available in all Match variants is vital.

comment:8 Changed 5 years ago by simonpj

I don't object to putting this fix in 7.10. I've looked at the patch.

Simon

comment:9 Changed 5 years ago by alanz

Milestone: 7.12.17.10.1
Priority: normalhighest

Updating milestone and priority to make 7.10.1

comment:10 Changed 5 years ago by alanz

Differential Rev(s): D639

comment:11 Changed 5 years ago by thoughtpolice

Differential Rev(s): D639Phab:D639

comment:12 Changed 5 years ago by thoughtpolice

Status: newpatch

comment:13 Changed 5 years ago by Austin Seipp <austin@…>

In c88e11219c1e914b71d8c630a1f1d4f6f0fb6b9b/ghc:

Bring Match m_fun_id_infix through the renamer.

Summary:
This is a first step for #9988

It turns out that bringing m_fun_id_infix through the renamer is
actually very simple, affecting the internals of rnMatch' only.

Is this simple enough to hit 7.10.1?

Test Plan: ./validate

Reviewers: hvr, simonpj, austin

Reviewed By: simonpj, austin

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D639

GHC Trac Issues: #9988

comment:14 Changed 5 years ago by thoughtpolice

Resolution: fixed
Status: patchclosed

Merged into HEAD and 7.10 - thanks!

comment:15 Changed 5 years ago by alanz

Thanks.

I will complete #10061 after GHC 7.10 is out, I am using this time to make sure the API Annotations infrastructure is usable, while there is still an (ever diminishing) chance to do something about it.

comment:16 Changed 4 years ago by Ben Gamari <ben@…>

In f0f9365/ghc:

Remove fun_infix from Funbind, as it is now in Match

One of the changes D538 introduced is to add `m_fun_id_infix` to `Match`

```lang=hs
data Match id body
  = Match {
        m_fun_id_infix :: (Maybe (Located id,Bool)),
          -- fun_id and fun_infix for functions with multiple equations
          -- only present for a RdrName. See note [fun_id in Match]
        m_pats :: [LPat id], -- The patterns
        m_type :: (Maybe (LHsType id)),
                                 -- A type signature for the result of the match
                                 -- Nothing after typechecking
        m_grhss :: (GRHSs id body)
  } deriving (Typeable)
```

This was done to track the individual locations and fixity of the
`fun_id` for each of the defining equations for a function when there
are more than one.

For example, the function `(&&&)` is defined with some prefix and some
infix equations below.

```lang=hs
    (&&&  ) [] [] =  []
    xs    &&&   [] =  xs
    (  &&&  ) [] ys =  ys
```

This means that the fun_infix is now superfluous in the `FunBind`. This
has not been removed as a potentially risky change just before 7.10 RC2,
and so must be done after.

This ticket captures that task, which includes processing these fields
through the renamer and beyond.

Ticket #9988 introduced these fields into `Match` through renaming, this
ticket it to continue through type checking and then remove it from
`FunBind` completely.

The split happened so that #9988 could land in 7.10

Trac ticket : #10061

Test Plan: ./validate

Reviewers: goldfire, austin, simonpj, bgamari

Reviewed By: bgamari

Subscribers: simonpj, thomie, mpickering

Differential Revision: https://phabricator.haskell.org/D1285

GHC Trac Issues: #10061
Note: See TracTickets for help on using tickets.