-
Notifications
You must be signed in to change notification settings - Fork 26
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 array access #832
fix array access #832
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks straightforward enough (especially when reviewed with ?w=1
). I only have a minor comment on the error message, but I would be fine with merging it anyway. Will merge this afternoon unless anyone objects.
src/types/Typechecker/Typechecker.hs
Outdated
typeArguments = typeArgs} | ||
else do | ||
tcError $ | ||
ExpectingOtherTypeError "an array access or a function call" ty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Should this be "an array or a function"
? This way the error message becomes
Expected an array or a function but found expression of type 'int'
reflecting that the type error is not the whole expression, but only the sub-expression that is being called.
thanks! I have remove the |
Have you also pushed? |
@@ -0,0 +1 @@ | |||
Expected an array access or a function call but found expression of type 'int' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test needs to be updated as well
It seems that |
src/types/Typechecker/Typechecker.hs
Outdated
Nothing -> tcError $ WrongNumberOfFunctionArgumentsError | ||
qname (length argTypes) (length args) | ||
unless (isArrowType ty) $ | ||
tcError $ NonFunctionTypeError ty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the thing that could go according to @albertnetymk. I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch @albertnetymk and @EliasC. Fixed!
OK, then it makes sense to restructure the flow to be sth like the following to "return early" for abnormal case, IMO. unless (isArrowType ty) $
error
-- normal case follows |
@albertnetymk I thought about it and considered that the majority of the time the programmer knows that an array is an array, so I was favouring the other pattern, where the developer is correct in most occasions. I have no problems going for your suggestion though. (@albertnetymk it would be great if your comments are actually part of the review or marked in the appropriate lines, as the ones from @EliasC) |
I agree with Kiko. Having the failing case last makes sense to me. |
By "return early", a level of indentation is removed, which is considered a decrease in certain complexity metrics. Surely, this is very subjective; the author should make the judgement call. |
The same "complexity reduction" could be achieved by writing
instead of
|
I don't think that's true in Haskell, because |
This is a matter of taste and style. We all have provided good reasons for choosing one option or the other. Can we please move past the |
I took an executive decision and merged this =) |
The "early return" is about a taste of preference, but the indentation discussion is not. My previous statement, "
|
Perfect valid Haskell code ≠ accepted code in Encore compiler |
This commit fixes accesses to an array when the type is different from an array; closes #823 and #831
This test case used to crash the compiler
With this commit, the compiler informs you that it expects an array of a function call and you gave it an int type.