Opened 15 months ago

Closed 10 months ago

Last modified 5 weeks ago

#9230 closed feature request (fixed)

Make -fwarn-tabs the default

Reported by: dfeuer Owned by: mlen
Priority: normal Milestone: 7.10.1
Component: Compiler Version: 7.8.2
Keywords: Cc: hvr, asr, oerjan
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions: Phab:D255, Phab:D399

Description

Tabs trip up an awful lot of Haskell beginners. Turning the warning on by default will likely save them a lot of grief. Any insanehhhhhhexperienced Haskellers who like tabs will have no trouble disabling the warning in their pragmas.

Change History (35)

comment:1 Changed 15 months ago by goldfire

+1 from me.

comment:2 Changed 15 months ago by tibbe

I've been bitten by files that mixed tabs and spaces before, so +1 from me.

comment:3 Changed 14 months ago by jstolarek

  • Owner set to jstolarek

comment:4 Changed 14 months ago by dfeuer

I looked into what making -fwarn-tabs the default would break in the GHC tree, and the answer appears to be "very little". It looks like the only files that use tabs for layout, use -Werror, and don't specify -fno-warn-tabs are libraries/time/Test/{ShowDST.hs,TimeZone.hs,CurrentTime.hs} and a test file that's designed to break from the tab warning. So contrary to some initial fears thoughtpolice raised in IRC, it will *not* be necessary to detab the whole GHC source to make this change.

comment:5 Changed 12 months ago by mlen

  • Owner changed from jstolarek to mlen

comment:6 Changed 12 months ago by mlen

I implemented this feature and a test case. Implementation took only few lines of code, but in order compile the compiler I needed to add quite a lot GHC_OPTIONS pragmas with -fno-warn-tabs (few git submodules were affected by this change). I have a week off and I won't have access to my computer, but I'll continue working on the patch when I get back.

comment:7 Changed 12 months ago by jstolarek

Where can we see the patch?

BTW. Thanks for taking over this. It slipped from my radar completely.

comment:8 Changed 12 months ago by mlen

The main patch can be found here: https://github.com/mlen/ghc/commit/62cd7b983aee6b581f42184fcc38b3d9bf50780e, while this commit includes only additions of -fno-warn-tabs: https://github.com/mlen/ghc/commit/1c60eaffe373bf6a6324d1a243130b9dbbf843b5 I still need to take care of submodules. libraries/{haskeline,hpc,stm,time,unix,xhtml} and utils/hsc2hs need to be patched because of -Werror.

comment:9 Changed 12 months ago by jstolarek

I think it is a good idea to also add a comment that motivates to remove tabs when doing some major changes to the file. I think we could just reuse the comment used in GHC source code:

{-# OPTIONS_GHC -fno-warn-tabs #-}
-- The above warning supression flag is a temporary kludge.
-- While working on this module you are encouraged to remove it and
-- detab the module (please do the detabbing in a separate patch). See
--     http://ghc.haskell.org/trac/ghc/wiki/Commentary/CodingStyle#TabsvsSpaces
-- for details

comment:10 Changed 12 months ago by mlen

I added the comment(s) and rebased over master. Will continue to work on patches for submodules during the weekend.

comment:11 Changed 12 months ago by hvr

  • Cc hvr added
  • Milestone set to 7.10.1

comment:12 Changed 11 months ago by asr

  • Cc asr added

comment:13 Changed 11 months ago by hvr

I've emailed the developers whose name show up in Phab:P22 suggesting to remove tabs in the modules they had touched last according to Git. Hopefully, the Haskell modules in ghc.git will be tab-free soon...

comment:14 Changed 11 months ago by mlen

Great to hear that. I prepared the patches to suppress tab warnings in the submodules that caused troubles. Now I got stuck while de-tabbing the testsuite, as some tests won't pass. For example there are errors about wrong std{out,err} in should_compile (and others), but I should get that working soon.

comment:15 Changed 11 months ago by mlen

During HacBerlin I discussed about the issue with Andres and he suggested an alternative approach: instead of detabbing or suppressing the warnings per file, a global suppression flag should be added.

It turned out to be very simple to implement. The change can be found here: https://github.com/mlen/ghc/compare/warn-tabs

I also continued the detabbing and the patch got very big (https://github.com/mlen/ghc/compare/detabbed), so there may cause trouble when merging. It still doesn't cover all tests (some of them are not run on my machine) and doesn't contain patches for submodules.

Is there any reason why the code from https://github.com/mlen/ghc/compare/warn-tabs branch shouldn't be used instead?

comment:16 follow-up: Changed 11 months ago by simonpj

I'm sorry, I'm quite confused. GHC already has a -fwarn-tabs flag, doesn't it? There are two patches offered above, but I'm not clear what the purpose of either is. Maybe I have not ready through this ticket carefully enough.

Would someone just like to write down what the issue is, what the alternatives are, their pros and cons, and which of the alternatives they recommend following.

I have no opinion myself; I'd just like see this resolved.

Simon

comment:17 in reply to: ↑ 16 Changed 11 months ago by dfeuer

Replying to simonpj:

I'm sorry, I'm quite confused. GHC already has a -fwarn-tabs flag, doesn't it? There are two patches offered above, but I'm not clear what the purpose of either is. Maybe I have not ready through this ticket carefully enough.

I don't understand that either. The essential problem we have is that we want -fwarn-tabs to be the default, but we have to find some way to get validation to pass when there are currently a lot of libraries in the GHC tree that have tabs but don't invoke -fno-warn-tabs.

comment:18 follow-ups: Changed 11 months ago by thoughtpolice

The above patch does all of the above correctly; it is also on Phabricator - see Phab:D255 (which also built correctly).

In essence it just adds Opt_WarnTabs (AKA -fwarn-tabs) to standardWarnings in DynFlags, which is the correct move. Next, it disables -fno-warn-tabs when building the libraries by modifying mk/validate-settings.mk to include -fno-warn-tabs, so that everything can validate correctly.

I need to do a review on the testsuite changes in the patch, but it seems fine at a quick glance (I mostly wonder about the changes to the testsuite driver).

comment:19 Changed 11 months ago by thoughtpolice

  • Differential Revisions set to Phab:D255

comment:20 in reply to: ↑ 18 Changed 11 months ago by dfeuer

Replying to thoughtpolice:

In essence it just adds Opt_WarnTabs (AKA -fwarn-tabs) to standardWarnings in DynFlags, which is the correct move. Next, it disables -fno-warn-tabs when building the libraries by modifying mk/validate-settings.mk to include -fno-warn-tabs, so that everything can validate correctly.

This may very well be the best thing for right now, but here's another idea that would take more work but might pay off in the long run, especially if other warnings are added to the defaults in the future. Instead of actually running ghc with -Werror, make the validation script watch for warnings. That is, replace an Error monad with a Writer monad. Given appropriate options, validation could collect warnings instead of halting immediately.

This would allow us to automatically create lists of files for which specific warnings should be tolerated. Obviously, these lists would need to be maintained in order to prevent regressions, but this is easy: Phabricator (or whatever) would collect a "warning diff" from each build that passes validation, which could be used to update the lists. Ideally, we'd also find a way to trim down the warning list generated by a differential to just those files it changed, so as to get notice that something else might need to be fixed.

comment:21 in reply to: ↑ 18 Changed 11 months ago by mlen

Sorry, I didn't mean to confuse anyone. I should've added the link to phabricator as soon as I pushed to it, I guess.

Replying to thoughtpolice:

I need to do a review on the testsuite changes in the patch, but it seems fine at a quick glance (I mostly wonder about the changes to the testsuite driver).

The purpose of changes in test driver was to make it possible to remove compile time flag. Test T9230 wouldn't make any sense if it was run with -fno-warn-tabs, so I had to remove that flag for this test.

I hope it is still possible for this patch to be released with GHC 7.10.

comment:22 Changed 11 months ago by mlen

  • Status changed from new to patch

comment:23 Changed 11 months ago by Austin Seipp <austin@…>

In 972ba1210d1bb535c41526ce396c0f086d30b59a/ghc:

Enabled warn on tabs by default (fixes #9230)

Summary:
This revision enables -fwarn-tabs by default and add a suppression
flag, so that GHC compilation won't fail when some files contain tab
characters.

Test Plan: Additional test case, T9230, was added to cover that change.

Reviewers: austin

Reviewed By: austin

Subscribers: simonmar, ezyang, carter, thomie, mlen

Differential Revision: https://phabricator.haskell.org/D255

GHC Trac Issues: #9230

Conflicts:
	testsuite/driver/testlib.py

comment:24 Changed 10 months ago by oerjan

  • Cc oerjan added

This is an excellent change, but if it is to be newbie-friendly (which I ideally and egoistically define as "telling the user enough not to make yet another confused post on Stackoverflow"), maybe it could use a more explanatory error message than "Warning: Tab character"? Something like

Tab characters aren't displayed portably. They can cause code to look correctly indented when it isn't, or vice versa, wildly varying dependent on the setup of the editor/browser/web site etc. used to view it.

comment:25 Changed 10 months ago by thomie

I don't think this patch as is stands is Python3 compabible. Please see my inline comments in https://phabricator.haskell.org/rGHC972ba1210d1bb535c41526ce396c0f086d30b59a

comment:26 Changed 10 months ago by thomie

  • Status changed from patch to infoneeded

comment:27 Changed 10 months ago by mlen

  • Differential Revisions changed from Phab:D255 to Phab:D255, Phab:D399
  • Status changed from infoneeded to patch

Proposed patch @ Phab:D399

comment:28 Changed 10 months ago by Austin Seipp <austin@…>

In c6d4ae6f437fb041ea70f3d2b4f7f0d03ff797bf/ghc:

Fix test driver python3 compatibility issues

Summary:
Fixes python3 compatibility issues by replacing filter with a list
comperhension and a potential issue with python2 when override_flags
would be an empty list.

Reviewers: austin, thomie

Reviewed By: austin, thomie

Subscribers: thomie, carter, simonmar, mlen

Differential Revision: https://phabricator.haskell.org/D399

GHC Trac Issues: #9230

comment:29 Changed 10 months ago by thoughtpolice

  • Resolution set to fixed
  • Status changed from patch to closed

Merged, thanks!

comment:30 follow-up: Changed 10 months ago by Polarina

Tabs are generally not the cause for incorrect indentation. It is mixing them with spaces that do.

An alternative approach would be to warn if a tab character is preceded by a space. That would keep the tabs people happy, even those that use tabs for indentation and spaces for alignment; while catching most cases of tab/space mix-ups.

comment:31 Changed 10 months ago by isaacdupree

re: Polarina: Let's try describing a rule that would catch all suspect uses of tabs.

Tabs may not be preceded by a non-tab character: that's a start.

Let's add: If two adjacent lines have different numbers of tabs, the line with fewer tabs must have a non-whitespace character following the last tab. This way the layout algorithm will never compare the width of a space and a tab (I think). Except that is not enough when layout is begun on the same line as the layout-inducing keyword:

f x = z where y = x + x
              z = y + y

(Personally I consider it bad style to do that even with spaces. If you change the first line before the layout keyword, you need to change the whole layout. Also if you dare to use a non-monospace font it will look wrong. I prefer starting the layout on a new line. [I wish I had a warning for this.])

I think there is a way to warn exactly when the tab width affects layout, but it would have to happen after lexing, and I'm not volunteering to implement it.

comment:32 in reply to: ↑ 30 Changed 10 months ago by dfeuer

Replying to Polarina:

Tabs are generally not the cause for incorrect indentation. It is mixing them with spaces that do.

An alternative approach would be to warn if a tab character is preceded by a space. That would keep the tabs people happy, even those that use tabs for indentation and spaces for alignment; while catching most cases of tab/space mix-ups.

I don't think that will do the trick, but this ticket's comments aren't the right venue for the discussion. If you have an idea for improving -fwarn-tabs, you should open a new ticket.

comment:33 follow-up: Changed 4 months ago by beroal

What about people who use tab characters for indentation regularly? My code looks correct for any tab character width chosen.

comment:34 in reply to: ↑ 33 Changed 4 months ago by oerjan

Replying to beroal:

What about people who use tab characters for indentation regularly? My code looks correct for any tab character width chosen.

People who are sure they know what they're doing can use the -fno-warn-tabs flag.

The reason for making this the default is that tabs trip up a lot of people who are new to Haskell. It also makes their pleas for help extremely confusing and time-wasting on a site like Stack Overflow, which autoconverts tabs to spaces - with a different width rule than Haskell uses.

comment:35 Changed 5 weeks ago by yokto

Last edited 5 weeks ago by yokto (previous) (diff)
Note: See TracTickets for help on using tickets.