Skip to content
This repository has been archived by the owner on Jul 13, 2020. It is now read-only.

Enable -fdefer-type-errors on start-up #495

Merged
merged 3 commits into from
Dec 2, 2017
Merged

Conversation

chrisdone
Copy link
Collaborator

@chrisdone chrisdone commented Dec 1, 2017

See https://github.com/commercialhaskell/intero/issues/476#issuecomment-348467874 for detailed explanation.

I'm going to spend the day on this branch; if it doesn't cause any regressions in behavior (or bad performance), I'll merge to master.

If you want to test this out now, just run killall intero and start the processes up again.

@chrisdone
Copy link
Collaborator Author

@purcell Are you aware of any performance numbers showing that this is not usable? I'm going to test it on a few real world big files today and report if it's a bad experience.

@chrisdone
Copy link
Collaborator Author

Loading Types.hs from my Duet project (654 lines of types and derivings):

Normal With deferred type errors
13.658 seconds 13.329 seconds

No actual observed difference here.

@chrisdone
Copy link
Collaborator Author

chrisdone commented Dec 1, 2017

From Server.hs from a client project,

Situation Normal With deferred type errors
Type error 0.354 seconds 0.813 seconds
No type error 0.477 seconds 0.515 seconds

So the numbers are slower when there's more code going on. Flagging up a type error is more than twice as slow, and flagging that the code is OK takes an extra 100ms.

@purcell

@chrisdone
Copy link
Collaborator Author

chrisdone commented Dec 1, 2017

It seems to vary based on caching or garbage collection inside the GHC API, because it goes as low as

0.643 seconds

So, really, roughly twice as slow at picking up type errors. You basically pay the price of doing a full type check every time you request a :load, whether it's a type error or not.

In some cases it's consistently much more!

1.468 seconds  

versus without -fdefer-type-errors

0.267 seconds

If I insert an error further down in the file.

Warnings regularly get generated when there's an error in your code,
such as "not in use" for a thing with a bad type and other nonsense
results. It's kind of distracting, so instead like regular GHC behavior,
this defers showing warnings until there are no errors anymore.
@purcell
Copy link
Contributor

purcell commented Dec 2, 2017

I feel like it's probably workable. It's a little slower for me on some files which were often annoyingly slow to flycheck anyway.

Copy link
Contributor

@purcell purcell left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@@ -737,8 +738,10 @@ CHECKER and BUFFER are added to each item parsed from STRING."
(msg (match-string 3)) ;; Replace gross bullet points.
(type (cond ((string-match "^Warning:" msg)
(setq msg (replace-regexp-in-string "^Warning: *" "" msg))
(if (string-match "^\\[-Wdeferred-type-errors\\]" msg)
'error
(if (or (string-match "^\\[-Wdeferred-type-errors\\]" msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just use string-prefix-p here.

@chrisdone
Copy link
Collaborator Author

In the interest of always "working" (or, working more often), I think I'll merge.

The speed hit does bother me a bit, but we can always be cleverer later to optimize the speed.

@chrisdone chrisdone merged commit d16d308 into master Dec 2, 2017
@chrisdone chrisdone deleted the defer-type-errors branch December 2, 2017 11:49
@purcell
Copy link
Contributor

purcell commented Dec 4, 2017

After using this for a day or two, I reckon this has actually made the common case of flycheck error reporting significantly worse: if you refer to an unimported function, for example, then deferred errors will be reported in the code which calls it, but the root problem itself will not be detected. I feel like reverting this might be the best course of action in the first time.

@chrisdone
Copy link
Collaborator Author

@purcell I think I pushed a change to fix that - can you try latest master?

I noticed the same problem; out of scope identifiers become typed holes with deferred type errors on.

Can you still reproduce your problem?

@purcell
Copy link
Contributor

purcell commented Dec 4, 2017

Yeah, I think it's better now. Will plug away and report back. :-)

@chrisdone
Copy link
Collaborator Author

@purcell Cool!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants