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

Warn on misleading Type(..) imports #1398

Merged
merged 3 commits into from
Aug 19, 2015
Merged

Conversation

garyb
Copy link
Member

@garyb garyb commented Aug 17, 2015

Resolves #1391.

@paf31 I can't figure out why the warning doesn't have position info though. It's probably something silly, can you take a look?


withPosition :: SourceSpan -> ErrorMessage -> ErrorMessage
withPosition pos (PositionedError _ err) = withPosition pos err
withPosition pos err = PositionedError pos err
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is this right? It's what we had before, but if we're going top down surely it would be better to keep the original position, as it will be more specific.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we should always get the deepest position information we can.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it to do that then and see what effect it has, we can always revert it before releasing if it is worse.

@paf31
Copy link
Contributor

paf31 commented Aug 17, 2015

Is it definitely hitting the PositionedDeclarationRef case?

Looks good otherwise 👍

@garyb
Copy link
Member Author

garyb commented Aug 17, 2015

I guess that would be the sensible explanation for why it's not... perhaps they're getting mapped out somewhere. I'll look at it again tomorrow, just thought I'd have a quick go at this one before signing off for the night.

@garyb garyb force-pushed the 1391-misleading-type-import branch from 5a08a97 to 3a1bbc4 Compare August 18, 2015 11:32
@garyb
Copy link
Member Author

garyb commented Aug 18, 2015

😭

I'll get the tests working on Windows before attempting to "fix" this any further.

@garyb garyb force-pushed the 1391-misleading-type-import branch from 73d56d0 to f2aed3e Compare August 18, 2015 23:18
@@ -529,6 +529,7 @@ resolveImport currentModule importModule exps imps impQual =
isHidden hidden ref@(TypeRef _ _) =
let
checkTypeRef _ True _ = True
checkTypeRef r acc (PositionedDeclarationRef _ _ h) = checkTypeRef r acc h
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paf31 this not existing was the cause of my woes, previously checkedRefs below was deleting PositionedDeclarationRef from the AST which meant this case wasn't needed...

garyb added a commit that referenced this pull request Aug 19, 2015
@garyb garyb merged commit f1d159a into master Aug 19, 2015
@garyb garyb deleted the 1391-misleading-type-import branch August 19, 2015 09:02
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.

None yet

2 participants