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

Parser does not detect field initialization in a constructor despite exhaustive match clauses #3615

Closed
stefandd opened this issue Aug 21, 2020 · 5 comments · Fixed by #4006
Closed
Assignees

Comments

@stefandd
Copy link
Contributor

stefandd commented Aug 21, 2020

primitive Foo
    fun apply() : (U32 | None) => 1

type Bar is (U32 | None)

actor Main
    let bar : Bar

    new create(env : Env) =>
        match Foo()
        | let result : U32 =>
            bar = result
        | None =>
            bar = 0
        end

The field bar should have been successfully initialized in create() but the compiler complains about a '7:5: field left undefined in constructor' error

@SeanTAllen SeanTAllen added bug help wanted Extra attention is needed labels Aug 25, 2020
@SeanTAllen
Copy link
Member

We discussed this during sync and @jemc thinks this isn't a huge amount of work but also that it isn't trivial. His fear is that exhaustive match checking happens after the initialization check (which would make this somewhat difficult).

@SeanTAllen
Copy link
Member

SeanTAllen commented Jan 29, 2022

"field left undefined in constructor" is currently done in the refer pass in refer_new.

exhaustive match is checked in is_match_exhaustive in the expression pass.

So we have a problem of ordering. If the exhaustive-ness match happened first this should be fine.

@SeanTAllen SeanTAllen added the good first issue Good for newcomers label Jan 29, 2022
@SeanTAllen
Copy link
Member

The specific check that fails in refer_new is (status != SYM_DEFINED)).

@SeanTAllen SeanTAllen removed the good first issue Good for newcomers label Jan 29, 2022
@SeanTAllen
Copy link
Member

@jemc I feel like the correct thing do here is:

  • move refer_new checks to verify pass.
  • let the exhaustiveness work in expression pass set up the ast correctly for exhaustiveness (no else with no definition)
  • run the equiv of refer_match again after exhaustiveness to consolidate branches

if the last is done i think that should define the symbol, but I could be wrong.

Can you verify?

@jemc
Copy link
Member

jemc commented Jan 29, 2022

If you do so, I suggest putting the checks in a new pass by themselves, since if it works like the refer pass currently does, it will be a pretty extensive pass touching a lot of different areas, so I think it's better to keep it in a separate pass, away from the relatively small and isolated checks currently in the verify pass.

In the Savi compiler I wrote a pass named completeness which serves this purpose (although that one works a bit differently than the one in ponyc does)

The main downside of moving this stuff out of the refer pass is that much of the code in the refer pass would likely need to be duplicated. Although maybe you can find a nifty way to reuse the symbol status tracking code instead of copying it.

@SeanTAllen SeanTAllen removed the help wanted Extra attention is needed label Feb 14, 2022
@SeanTAllen SeanTAllen self-assigned this Feb 14, 2022
SeanTAllen added a commit that referenced this issue Feb 14, 2022
…tion

Previously, exhaustive matches were handled after we verified that an object's fields
were initialized. This lead to the code such as the following not passing compiler
checks:

```pony
primitive Foo
  fun apply() : (U32 | None) => 1

type Bar is (U32 | None)

actor Main
  let bar : Bar

  new create(env : Env) =>
      match Foo()
      | let result : U32 =>
          bar = result
      | None =>
          bar = 0
      end
```

This commit moves field initialization checking to the verify pass so that it
comes after the expression pass when exhaustiveness checking is done. This also
moves code to patchup matches from the refer pass to a new completeness pass
that comes after the expression pass. The new `completeness_match` code is
identical to `refer_match` code except that an else clause with a value of
`TK_NONE` no longer means a clause exists. In the exhaustiveness checks in the
expression pass, any non-exhaustive else clause that was a `TK_NONE` has since
been replaced with a `else None` so, by the time we get to completeness,
`TK_NONE` literally means "there's no else clause".

Closes #3615
SeanTAllen added a commit that referenced this issue Feb 14, 2022
…tion

Previously, exhaustive matches were handled after we verified that an object's fields
were initialized. This lead to the code such as the following not passing compiler
checks:

```pony
primitive Foo
  fun apply() : (U32 | None) => 1

type Bar is (U32 | None)

actor Main
  let bar : Bar

  new create(env : Env) =>
      match Foo()
      | let result : U32 =>
          bar = result
      | None =>
          bar = 0
      end
```

This commit moves field initialization checking to the verify pass so that it
comes after the expression pass when exhaustiveness checking is done. This also
moves code to patchup matches from the refer pass to a new completeness pass
that comes after the expression pass. The new `completeness_match` code is
identical to `refer_match` code except that an else clause with a value of
`TK_NONE` no longer means a clause exists. In the exhaustiveness checks in the
expression pass, any non-exhaustive else clause that was a `TK_NONE` has since
been replaced with a `else None` so, by the time we get to completeness,
`TK_NONE` literally means "there's no else clause".

Closes #3615
SeanTAllen added a commit that referenced this issue Feb 14, 2022
…tion

Previously, exhaustive matches were handled after we verified that an object's fields
were initialized. This lead to the code such as the following not passing compiler
checks:

```pony
primitive Foo
  fun apply() : (U32 | None) => 1

type Bar is (U32 | None)

actor Main
  let bar : Bar

  new create(env : Env) =>
      match Foo()
      | let result : U32 =>
          bar = result
      | None =>
          bar = 0
      end
```

This commit moves field initialization checking to the verify pass so that it
comes after the expression pass when exhaustiveness checking is done. This also
moves code to patchup matches from the refer pass to a new completeness pass
that comes after the expression pass. The new `completeness_match` code is
identical to `refer_match` code except that an else clause with a value of
`TK_NONE` no longer means a clause exists. In the exhaustiveness checks in the
expression pass, any non-exhaustive else clause that was a `TK_NONE` has since
been replaced with a `else None` so, by the time we get to completeness,
`TK_NONE` literally means "there's no else clause".

Closes #3615
SeanTAllen added a commit that referenced this issue Feb 15, 2022
…tion

Previously, exhaustive matches were handled after we verified that an object's fields
were initialized. This lead to the code such as the following not passing compiler
checks:

```pony
primitive Foo
  fun apply() : (U32 | None) => 1

type Bar is (U32 | None)

actor Main
  let bar : Bar

  new create(env : Env) =>
      match Foo()
      | let result : U32 =>
          bar = result
      | None =>
          bar = 0
      end
```

This commit moves field initialization checking to the verify pass so that it
comes after the expression pass when exhaustiveness checking is done. This also
moves code to patchup matches from the refer pass to a new completeness pass
that comes after the expression pass. The new `completeness_match` code is
identical to `refer_match` code except that an else clause with a value of
`TK_NONE` no longer means a clause exists. In the exhaustiveness checks in the
expression pass, any non-exhaustive else clause that was a `TK_NONE` has since
been replaced with a `else None` so, by the time we get to completeness,
`TK_NONE` literally means "there's no else clause".

Closes #3615
SeanTAllen added a commit that referenced this issue Feb 15, 2022
…tion (#4006)

Previously, exhaustive matches were handled after we verified that an object's fields
were initialized. This lead to the code such as the following not passing compiler
checks:

```pony
primitive Foo
  fun apply() : (U32 | None) => 1

type Bar is (U32 | None)

actor Main
  let bar : Bar

  new create(env : Env) =>
      match Foo()
      | let result : U32 =>
          bar = result
      | None =>
          bar = 0
      end
```

This commit moves field initialization checking to the verify pass so that it
comes after the expression pass when exhaustiveness checking is done. This also
moves code to patchup matches from the refer pass to a new completeness pass
that comes after the expression pass. The new `completeness_match` code is
identical to `refer_match` code except that an else clause with a value of
`TK_NONE` no longer means a clause exists. In the exhaustiveness checks in the
expression pass, any non-exhaustive else clause that was a `TK_NONE` has since
been replaced with a `else None` so, by the time we get to completeness,
`TK_NONE` literally means "there's no else clause".

Closes #3615
@SeanTAllen SeanTAllen removed the bug label Jan 22, 2025
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 a pull request may close this issue.

3 participants