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

Fix #292: adding tuple in hasSameKind function in Type.hs #296

Merged
merged 4 commits into from
Dec 14, 2015
Merged

Fix #292: adding tuple in hasSameKind function in Type.hs #296

merged 4 commits into from
Dec 14, 2015

Conversation

PhucVH888
Copy link
Contributor

This change fixes the issue #292.

@albertnetymk
Copy link
Contributor

I think it's better to add a test case to avoid regression.

@PhucVH888
Copy link
Contributor Author

I agree with you. Here is the test case:

passive class Foo {
  t : (int,int)

  def foo() : Maybe (int,int)
    Just this.t
}

class Main
  def main() : void
    ()

I previously fixed the issue by inserting areBoth isTupleType || to areBoth isPrimitive ||, then the issue is fixed.

However, before committing to the plenary, I moved the code to areBoth isMaybeType ||, and I found that another issue as the following:

phuc:workspace vo$ encorec test1.enc
Importing module String from /Users/vo/code/encore/bundles/standard/String.enc
encorec: No match in record selector resultType

@EliasC : do you know why?

@EliasC
Copy link
Contributor

EliasC commented Dec 2, 2015

@vhphuc: If you move code around in a function, you should generally expect different results! What we want is to say in this particular function is "if both types are tuples, return True", which is what your original fix did. The code in this commit is instead saying "if both types are tuples, check that the result types of both types are of the same kind", which fails as tuples do not have a result type (more technically, there is no field resultType in the constructor TupleType).

Also: Don't forget to add your test case to the test suite! You will probably want to add a print statement to the main method so that you can check that the program compiles and runs as expected (look at the other test cases in src/tests/encore/basic/ for inspiration).

@PhucVH888
Copy link
Contributor Author

@EliasC: You are right. I found that there is no field resultType in typeMap. I will prepare the test case and place it in src/tests/encore/basic/. Tack!

@EliasC
Copy link
Contributor

EliasC commented Dec 11, 2015

I have two issues with the test case:

  • There is commented code in the file that should be removed.

  • Many functions running the test cases assumes that a certain value is being passed in. It would be easier to read if each function was self-contained, e.g.

    def testFoo() {
      let t = (32, 32) in
        match t with
          (32, 32) => print "success"
          _ => print "fail"
    }
    

    rather than

    def testFoo(t : (int, int)) {
        match t with
          (32, 32) => print "success"
          _ => print "fail"
    }
    

    Of course some functions are testing that arguments can be passed and returned, and in that case you cannot do it in the first way.

After fixing this, I think this could be merged!

@PhucVH888
Copy link
Contributor Author

For some functions, it is necessary to maintain arguments in case they can passed and returned. With unnecessary arguments in other functions, I omitted already. Ready to commit.

@EliasC
Copy link
Contributor

EliasC commented Dec 11, 2015

Ready to commit.

Does this mean that you will be pushing additional material to this PR?

@PhucVH888
Copy link
Contributor Author

Not really push additional material, but I fix the two aforementioned issues.

@EliasC
Copy link
Contributor

EliasC commented Dec 12, 2015

Personally, I would change some more functions according to my latest comment, but I think this is ready to be merged. Will merge later if no one objects.

EliasC added a commit that referenced this pull request Dec 14, 2015
Fix #292: adding tuple in `hasSameKind` function in `Type.hs`
@EliasC EliasC merged commit 33612d1 into parapluu:features/plenary Dec 14, 2015
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.

4 participants