Experiences from the SIP 18 conversion

[martin odersky]

I just finished converting the Scala standard library and the Scala compiler to avoid feature warnings as they are prescribed by SIP 18. Overall it was fairly smooth; it took about 4 hours net for over 1200 source files. I spent more time fine-tuning the detection process and reporting than changing files or flags. I went from a warnings everywhere policy to a policy much like deprecated and unchecked, where you get only a summary of the number of feature warnings unless you compile with -feature. That gives you yet another way to simply ignore the issue if you so choose.

Doing the changes to the codebase gave me a good idea how our current Scala code fares wrt the categories in the SIP. And that, in turn, gave some info how useful the feature warnings are. 

In terms of number of warnings the large majority of all warnings were about implicit conversions and postfix operators. Implicit conversions in the library were 

pretty much as expected. I was a but surprised how few of them could be eliminated by implicit classes (maybe 10%). A lot of implicit conversions in the compiler were pretty dubious. For instance I found about a dozen instances of implicit conversions from String to TermName in various parts of the compiler. Needless to say, that's terrible style, and we need to fix this. This shows clearly that implicit conversions are both very tempting and often dubious, so I think it's really useful to demand that the feature be explicitly enabled.

Postfix operators were also very common, and many usages were dubious. They were not incorrect but it just introduced unnessecary variation in coing style. Why write (xs nonEmpty) when in 99% of other usages you write xs.nonEmpty? There were also quite a few instances where people used a postfix operator followed by an explicit semicolon, because otherwise the operator would have been interpreted as infix. I found that abhorrent because it introduces a double irregularity in the syntactic style. So, in summary, I am glad postfix ops need to be enabled explicitly in the future. Hopefully that will have some homogenizing effect on Scala coding style.

These two made up the lions share of feature usages (maybe 80-90%). Of the other three features, higher-kinded types were the most common. They were mostly concentrated in some parts of collections and concurrency primitives. There was nothing wrong with any usage I saw, so the feature warning exists mostly as  a marker that higher kinded types are not as stable as the other parts of Scala. The 4th feature in terms of number of occurrences were existential types that cannot be represented as wildcard types. Most of these were inferred. I found it mostly useful to know that these fairly hairy types are generated because it makes one think about simpler alternatives. 

The feature that was (fortunately) least often used was reflective calls over structural types. About one in two these usages seemed to be unintended. So, enabling the feature explicitly is a clear win.

This discussion has excluded the new features that need to be enabled or else give an error. Of course, there were no usages of type Dynamic yet. There were a lot of usages of macros (most of them in a huge number of tests) that all needed to be adapted from -Xmacro to either an 'import language.experimental.macros'

or a flag '- language:experimental.macros'. Changing all these tests was a bit painful. I'm glad that's done now.

Cheers

 - Martin

[Paul Phillips]

On Sat, Apr 14, 2012 at 2:15 AM, martin odersky <martin....@epfl.ch> wrote:
> For instance I found about a dozen instances of implicit conversions from
> String to TermName in various parts of the compiler. Needless to say, that's
> terrible style, and we need to fix this.

A dozen, down from like 500 you know. And the implicit used to be
String => Name, which was even worse. (And as a final twist of the
casual usage knife, the method was called "view".) See:

https://github.com/scala/scala/commit/88a54be3877#L55R114
https://github.com/scala/scala/commit/765f9aa2bf
https://github.com/scala/scala/commit/6c04413edb

Also, in the nice "punches you don't fully appreciate until they land"
department, removing the implicit from String => TypeName broke sbt
sufficiently thoroughly (its compiler interface depending on this
apparently not-so-internal detail) that there has been no release
version of sbt which can build scala trunk since somewhere around
1977.

[martin odersky]

On Sat, Apr 14, 2012 at 1:10 AM, Eugene Burmako <eugene....@epfl.ch> wrote:

Hi Martin,

Since you've mentioned that changing macro tests was painful, could you, please, elaborate?

Is this the sheer amount of tests that caused trouble? Is this something else? How could I improve in the future?

It was simply the number of tests. What would recommend is to merge tests into larger files. That should also improve times spent testing which means people run tests more often, which should be good for everyone.

In fact that's it's not just macros but a lot of other tests as well that could be profitably merged.

Cheers

 - Martin

[Adriaan Moors]

the downside of larger tests is that they convey less information: what exactly do they test?

especially for type checker-related bugs, it's massively useful to have small tests,

or you immediately drown in -Ytyper-debug output 

if many test files pose practical problems, maybe tools can help?

partest's --update-check has already saved me a lot of time,

and I'm sure there's more we can improve in this area

cheers

adriaan

[Paul Phillips]

On Sat, Apr 14, 2012 at 6:55 PM, Adriaan Moors <adriaa...@epfl.ch> wrote:
> the downside of larger tests is that they convey less information: what
> exactly do they test?
> especially for type checker-related bugs, it's massively useful to have
> small tests,
> or you immediately drown in -Ytyper-debug output

Yes, smaller tests are better unless one is specifically setting out
to test many things at once, each of which should have a smaller test
which supports it individually. Engineering principles don't go out
the window when writing tests, either: cut and paste is exactly as
good an idea as it is everywhere else. For some ideas on how one
might avoid it, see examples:

% ls -1 src/partest/scala/tools/partest/*Test.scala
src/partest/scala/tools/partest/CompilerTest.scala
src/partest/scala/tools/partest/DirectTest.scala
src/partest/scala/tools/partest/IcodeTest.scala
src/partest/scala/tools/partest/ReplTest.scala
src/partest/scala/tools/partest/ScaladocModelTest.scala
src/partest/scala/tools/partest/SecurityTest.scala
src/partest/scala/tools/partest/SigTest.scala

Testing reification probably offers unique challenges, if abstracting
something means you are reifying the abstraction instead of what you
wanted to reify. But such an excuse should be very much the
exception.

If you guys want to see what a good test suite looks like, run git's
sometime. Pasting the transcript doesn't even do it justice because
it very effectively uses ansi color to highlight relevant portions.
I'll include some if it anyway. (It goes on for much, much longer
than this, all of similar quality.)

% make test
*** t0000-basic.sh ***
ok 1 - .git/objects should be empty after git init in an empty repo
ok 2 - .git/objects should have 3 subdirectories
ok 3 - success is reported like this
not ok 4 - pretend we have a known breakage # TODO known breakage
ok 5 - pretend we have fixed a known breakage (run in sub test-lib)
ok 6 - test runs if prerequisite is satisfied
ok 7 # skip unmet prerequisite causes test to be skipped (missing DONTHAVEIT)
ok 8 - test runs if prerequisites are satisfied
ok 9 # skip unmet prerequisites causes test to be skipped (missing
DONTHAVEIT of HAVEIT,DONTHAVEIT)
ok 10 # skip unmet prerequisites causes test to be skipped (missing
DONTHAVEIT of DONTHAVEIT,HAVEIT)
ok 11 - tests clean up after themselves
ok 12 - tests clean up even on failures
ok 13 - git update-index without --add should fail adding
ok 14 - git update-index with --add should succeed
ok 15 - writing tree out with git write-tree
ok 16 - validate object ID of a known tree
ok 17 - git update-index without --remove should fail removing
ok 18 - git update-index with --remove should be able to remove
ok 19 - git write-tree should be able to write an empty tree
ok 20 - validate object ID of a known tree
ok 21 - adding various types of objects with git update-index --add
ok 22 - showing stage with git ls-files --stage
ok 23 - validate git ls-files output for a known tree
ok 24 - writing tree out with git write-tree
ok 25 - validate object ID for a known tree
ok 26 - showing tree with git ls-tree
ok 27 - git ls-tree output for a known tree
ok 28 - showing tree with git ls-tree -r
ok 29 - git ls-tree -r output for a known tree
ok 30 - showing tree with git ls-tree -r -t
ok 31 - git ls-tree -r output for a known tree
ok 32 - writing partial tree out with git write-tree --prefix
ok 33 - validate object ID for a known tree
ok 34 - writing partial tree out with git write-tree --prefix
ok 35 - validate object ID for a known tree
ok 36 - put invalid objects into the index
ok 37 - writing this tree without --missing-ok
ok 38 - writing this tree with --missing-ok
ok 39 - git read-tree followed by write-tree should be idempotent
ok 40 - validate git diff-files output for a know cache/work tree state
ok 41 - git update-index --refresh should succeed
ok 42 - no diff after checkout and git update-index --refresh
ok 43 - git commit-tree records the correct tree in a commit
ok 44 - git commit-tree records the correct parent in a commit
ok 45 - git commit-tree omits duplicated parent in a commit
ok 46 - update-index D/F conflict
ok 47 - real path works as expected
ok 48 - very long name in the index handled sanely
# still have 1 known breakage(s)
# passed all remaining 47 test(s)
1..48
*** t0001-init.sh ***
ok 1 - plain
ok 2 - plain nested in bare
ok 3 - plain through aliased command, outside any git repo
not ok 4 - plain nested through aliased command # TODO known breakage
not ok 5 - plain nested in bare through aliased command # TODO known breakage
ok 6 - plain with GIT_WORK_TREE
ok 7 - plain bare
ok 8 - plain bare with GIT_WORK_TREE
ok 9 - GIT_DIR bare
ok 10 - init --bare
ok 11 - GIT_DIR non-bare
ok 12 - GIT_DIR & GIT_WORK_TREE (1)
ok 13 - GIT_DIR & GIT_WORK_TREE (2)
ok 14 - reinit
ok 15 - init with --template
ok 16 - init with --template (blank)
ok 17 - init with init.templatedir set
ok 18 - init --bare/--shared overrides system/global config
ok 19 - init honors global core.sharedRepository
ok 20 - init rejects insanely long --template
ok 21 - init creates a new directory
ok 22 - init creates a new bare directory
ok 23 - init recreates a directory
ok 24 - init recreates a new bare directory
ok 25 - init creates a new deep directory
ok 26 - init creates a new deep directory (umask vs. shared)
ok 27 - init notices EEXIST (1)
ok 28 - init notices EEXIST (2)
ok 29 - init notices EPERM
ok 30 - init creates a new bare directory with global --bare
ok 31 - init prefers command line to GIT_DIR
ok 32 - init with separate gitdir
ok 33 - re-init to update git link
ok 34 - re-init to move gitdir
ok 35 - re-init to move gitdir symlink
# still have 2 known breakage(s)
# passed all remaining 33 test(s)
1..35
*** t0002-gitfile.sh ***
ok 1 - initial setup
ok 2 - bad setup: invalid .git file format
ok 3 - bad setup: invalid .git file path
ok 4 - final setup + check rev-parse --git-dir
ok 5 - check hash-object
ok 6 - check cat-file
ok 7 - check update-index
ok 8 - check write-tree
ok 9 - check commit-tree
ok 10 - check rev-list
# passed all 10 test(s)
1..10
*** t0003-attributes.sh ***
ok 1 - setup
ok 2 - command line checks
ok 3 - attribute test
ok 4 - attribute matching is case sensitive when core.ignorecase=0
ok 5 - attribute matching is case insensitive when core.ignorecase=1
ok 6 - check whether FS is case-insensitive
ok 7 - additional case insensitivity tests
ok 8 - unnormalized paths
ok 9 - relative paths
ok 10 - prefixes are not confused with leading directories
ok 11 - core.attributesfile
ok 12 - attribute test: read paths from stdin
ok 13 - attribute test: --all option
ok 14 - attribute test: --cached option
ok 15 - root subdir attribute test
ok 16 - setup bare
ok 17 - bare repository: check that .gitattribute is ignored
ok 18 - bare repository: check that --cached honors index
ok 19 - bare repository: test info/attributes
# passed all 19 test(s)
1..19
*** t0004-unwritable.sh ***
ok 1 - setup
ok 2 - write-tree should notice unwritable repository
ok 3 - commit should notice unwritable repository
ok 4 - update-index should notice unwritable repository
ok 5 - add should notice unwritable repository
# passed all 5 test(s)
1..5
*** t0005-signals.sh ***
ok 1 - sigchain works
# passed all 1 test(s)
1..1
*** t0006-date.sh ***
ok 1 - relative date (5 seconds ago)
ok 2 - relative date (5 minutes ago)
ok 3 - relative date (5 hours ago)
ok 4 - relative date (5 days ago)
ok 5 - relative date (3 weeks ago)
ok 6 - relative date (5 months ago)
ok 7 - relative date (1 year, 2 months ago)
ok 8 - relative date (1 year, 9 months ago)
ok 9 - relative date (20 years ago)
ok 10 - relative date (12 months ago)
ok 11 - relative date (2 years ago)
ok 12 - parse date (2008)
ok 13 - parse date (2008-02)
ok 14 - parse date (2008-02-14)
ok 15 - parse date (2008-02-14 20:30:45)
ok 16 - parse date (2008-02-14 20:30:45 -0500)
ok 17 - parse date (2008-02-14 20:30:45 -0015)
ok 18 - parse date (2008-02-14 20:30:45 -5)
ok 19 - parse date (2008-02-14 20:30:45 -5:)
ok 20 - parse date (2008-02-14 20:30:45 -05)
ok 21 - parse date (2008-02-14 20:30:45 -:30)
ok 22 - parse date (2008-02-14 20:30:45 -05:00)
ok 23 - parse date (2008-02-14 20:30:45 TZ=EST5)
ok 24 - parse approxidate (now)
ok 25 - parse approxidate (5 seconds ago)
ok 26 - parse approxidate (5.seconds.ago)
ok 27 - parse approxidate (10.minutes.ago)
ok 28 - parse approxidate (yesterday)
ok 29 - parse approxidate (3.days.ago)
ok 30 - parse approxidate (3.weeks.ago)
ok 31 - parse approxidate (3.months.ago)
ok 32 - parse approxidate (2.years.3.months.ago)
ok 33 - parse approxidate (6am yesterday)
ok 34 - parse approxidate (6pm yesterday)
ok 35 - parse approxidate (3:00)
ok 36 - parse approxidate (15:00)
ok 37 - parse approxidate (noon today)
ok 38 - parse approxidate (noon yesterday)
ok 39 - parse approxidate (last tuesday)
ok 40 - parse approxidate (July 5th)
ok 41 - parse approxidate (06/05/2009)
ok 42 - parse approxidate (06.05.2009)
ok 43 - parse approxidate (Jun 6, 5AM)
ok 44 - parse approxidate (5AM Jun 6)
ok 45 - parse approxidate (6AM, June 7, 2009)
# passed all 45 test(s)
1..45
*** t0010-racy-git.sh ***
ok 1 - Racy GIT trial #0 part A
ok 2 - Racy GIT trial #0 part B
ok 3 - Racy GIT trial #1 part A
ok 4 - Racy GIT trial #1 part B
ok 5 - Racy GIT trial #2 part A
ok 6 - Racy GIT trial #2 part B
ok 7 - Racy GIT trial #3 part A
ok 8 - Racy GIT trial #3 part B
ok 9 - Racy GIT trial #4 part A
ok 10 - Racy GIT trial #4 part B
# passed all 10 test(s)
1..10
*** t0020-crlf.sh ***
ok 1 - setup
ok 2 - safecrlf: autocrlf=input, all CRLF
ok 3 - safecrlf: autocrlf=input, mixed LF/CRLF
ok 4 - safecrlf: autocrlf=true, all LF
ok 5 - safecrlf: autocrlf=true mixed LF/CRLF
ok 6 - safecrlf: print warning only once
ok 7 - switch off autocrlf, safecrlf, reset HEAD
ok 8 - update with autocrlf=input
ok 9 - update with autocrlf=true
ok 10 - checkout with autocrlf=true
ok 11 - checkout with autocrlf=input
ok 12 - apply patch (autocrlf=input)
ok 13 - apply patch --cached (autocrlf=input)
ok 14 - apply patch --index (autocrlf=input)
ok 15 - apply patch (autocrlf=true)
ok 16 - apply patch --cached (autocrlf=true)
ok 17 - apply patch --index (autocrlf=true)
ok 18 - .gitattributes says two is binary
ok 19 - .gitattributes says two is input
ok 20 - .gitattributes says two and three are text
ok 21 - in-tree .gitattributes (1)
ok 22 - in-tree .gitattributes (2)
ok 23 - in-tree .gitattributes (3)
ok 24 - in-tree .gitattributes (4)
ok 25 - checkout with existing .gitattributes
ok 26 - checkout when deleting .gitattributes
ok 27 - invalid .gitattributes (must not crash)
ok 28 - setting up for new autocrlf tests
ok 29 - report no change after setting autocrlf
ok 30 - files are clean after checkout
ok 31 - LF only file gets CRLF with autocrlf
ok 32 - Mixed file is still mixed with autocrlf
ok 33 - CRLF only file has CRLF with autocrlf
ok 34 - New CRLF file gets LF in repo
# passed all 34 test(s)
1..34
*** t0021-conversion.sh ***
ok 1 - setup
ok 2 - check
ok 3 - expanded_in_repo
ok 4 - filter shell-escaped filenames
ok 5 - required filter success
ok 6 - required filter smudge failure
ok 7 - required filter clean failure
# passed all 7 test(s)
1..7

[Eugene Burmako]

now when we have eval and reporters/frontends exposed in the API, reify tests are just simple reify(...).eval, so they don't need any test harnesses, but it's certainly a good idea to merge them. same goes for macros. i was planning to do that along with performance testing after i'm done with the urgent stuff.