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

Map mode doesn't work anymore #86

Closed
melrief opened this issue Feb 1, 2014 · 11 comments
Closed

Map mode doesn't work anymore #86

melrief opened this issue Feb 1, 2014 · 11 comments
Assignees
Labels
Milestone

Comments

@melrief
Copy link
Collaborator

melrief commented Feb 1, 2014

I'm experiencing a strange bug in the develop branch: the map mode always hang hangs with the error:

> hawk -c /tmp/.hawk -m 'const $ "hello"'
error: Won't compile:
    Ambiguous occurrence `map'
It could refer to either `GHC.Base.map',
                         imported from `Prelude' (and originally defined in `base:GHC.Base')
                      or `Data.Vector.map', imported from `Data.Vector'

that's strange because we have System.Console.Hawk.Runtime.hs to avoid this kind of naming problems.

What's even more strange is that the tests don't fail with this error and that's unacceptable. We should check why we have this error and why the tests still work.

@gelisam
Copy link
Owner

gelisam commented Feb 1, 2014

Probably due to my refactoring, I shall investigate.

The kind of end-to-end tests (as opposed to unit tests) proposed by #26 would probably have caught that.

@gelisam
Copy link
Owner

gelisam commented Feb 1, 2014

Nope, not due to my refactoring since it works on refactor_options. Which branch did you notice this on?

@gelisam
Copy link
Owner

gelisam commented Feb 1, 2014

Also works on different_preludes. Do you have something special in your prelude?

@gelisam
Copy link
Owner

gelisam commented Feb 1, 2014

From your error message, I see that you have import Data.Vector in your user prelude. I can indeed reproduce your error in the different_prelude branch when I include that. Now I'm suspecting my refactoring once again; let me try on other branches.

@gelisam
Copy link
Owner

gelisam commented Feb 1, 2014

I can reproduce the issue on refactor_options and develop. Definitely my refactoring.

@gelisam
Copy link
Owner

gelisam commented Feb 1, 2014

Found it: in 148abd6, I tried to cut a corner in processSpec by converting all three modes into variants of --apply. That duplicated some of the work in the hawk function, but I missed the critical step of only using functions Runtime. I guess I forgot why we were doing that! I will fix it and add a comment explaining why we do it, so I don't make that mistake again.

@gelisam
Copy link
Owner

gelisam commented Feb 1, 2014

I no longer think that the tests in #26 would have caught the issue, because the problem wasn't with end-to-end testing but with a specific prelude contents. Now that we have -c, this kind of thing should be much easier to test!

gelisam added a commit that referenced this issue Feb 1, 2014
this duplicates code. more refactoring is needed.
@gelisam
Copy link
Owner

gelisam commented Feb 1, 2014

Fix pushed to develop.

@gelisam gelisam closed this as completed Feb 1, 2014
@gelisam
Copy link
Owner

gelisam commented Feb 1, 2014

Reopening until we also have a test to catch future regressions. Would you like to merge different_preludes to develop, so we can use -c in the test? You can then re-fork different_preludes after the merge if you want to continue working on it.

@gelisam gelisam reopened this Feb 1, 2014
@melrief
Copy link
Collaborator Author

melrief commented Feb 1, 2014

Done with 4bddd94

@gelisam
Copy link
Owner

gelisam commented Feb 9, 2014

I have added a test for this issue, but it's a bit different from the one you came up with. First, I have used Data.Set instead of Data.Vector because containers is installed by hawk as a dependency, but vector isn't. Second, I added a small input file, because a map without an input will hang while waiting for data from stdin.

> seq 3 | hawk -c tests/preludes/set -m 'const $ "hello"'
hello
hello
hello

I have verified that without d625793, the test fails because of the ambiguity between Prelude.map and Data.Set.map.

@gelisam gelisam closed this as completed Feb 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants