use travis CI to lint Phobos

[wilzbach]

follow-up to #4239. Here's a simple setup with Travis.
Obviously a lot of stuff fails (= Phobos doesn't stick to it's own style), but I argue that we should fix this and then enable this bot to help submitters.

Using DScanner allows us to check for a couple of goodies:

• Check for array literals that cause unnecessary allocation
• Check for poor exception handling practices
• Check for use of the deprecated 'delete' keyword
• Check for use of the deprecated floating point operators
• Check number literals for readability
• Checks that opEquals, opCmp, toHash, and toString are either const, immutable or inout.
• Checks for .. expressions where the left side is larger than the right.
• Checks for if statements whose 'then' block is the same as the 'else' block
• Checks for some problems with constructors
• Checks for unused variables and function parameters
• Checks for unused labels
• Checks for duplicate attributes
• Checks that opEquals and toHash are both defined or neither are defined
• Checks for methods or properties whose names conflict with built-in properties
• Checks for confusing code in inline asm statements
• Checks for undocumented public declarations_
• Checks for poor placement of function attributes
• Checks for use of the comma operator
• Checks for redundant expressions in if statements
• Checks for redundant parenthesis
• Checks for lines longer than 120 characters
• Checks for assignment to auto-ref function parameters
• Checks for incorrect infinite range definitions
• Checks for asserts that are always true

Ping @Hackerpilot.

For mir we already had to disable a couple of flags due to bugs or coding practices (thus they are not listed here) and I assume as Phobos is a lot bigger, we might need to flip one or other flag, but hey in the end we will have a bot that does all the nitpicking for us!

[Hackerpilot]

My parser hates index.d.

[wilzbach]

Great news: with #4245, #4246, #4247, the Travis passes. As mentioned I decreased the linting to a minimum, but now we do have it :)
Future work can fix more in the Phobos codebase - small low hanging fruits include e.g. > 120 lines or undocumented public methods.

Btw I found a small bug in libdparse, which the error "Primary expression expressed" is ignored.

[dnadlinger]

The file seems to have been hard-wrapped at a certain column count.

[wilzbach]

that's from the default config of Dscanner.
I beautified the two cases, were the hard-wrapping didn't work.
Thanks!

[Hackerpilot]

I blame @burner for the wrapping.

[PetarKirov]

Is this line needed?

[wilzbach]

Nope it was an artifact when I tried tests without Dscanner. Good spot!

[PetarKirov]

Nice work, @wilzbach! I'd love to see automated checking such as this leveraged during PR review.

BTW, it would also be useful as a Makefile rule for use during development. Maybe even required before building Phobos/Druntime/DMD/etc.

But let's first get this merged in.

[PetarKirov]

I reviewed all changes and everything LGTM.

[wilzbach]

I reviewed all changes and everything LGTM.

@ZombineDev They are submitted as separate PRs (#4245, #4246) because I didn't know how controversial they were. They are already toggled on auto merge, so as soon as win-farm-1 finds some time - I can rebase this PR and then we are good to go :)

Thanks a lot for your review & feedback!

[wilzbach]

Nice initiative. Thanks for doing this. Please stay on 4243 and merge it as soon as possible, so we don't have ripples following the style change from pulling other diffs. Thx! -- Andrei

All pending PRs are in, let's go?

[PetarKirov]

"All checks have passed" 🎉

[CyberShadow]

Auto-merge toggled on

[CyberShadow]

Well, since this has Andrei's blessing...

[wilzbach]

BTW, it would also be useful as a Makefile rule for use during development. Maybe even required before building Phobos/Druntime/DMD/etc.

This might be a bit more complicated as we would need to build dscanner then.
However in theory it should be dub fetch dscanner && dub run dscanner, but feel free to explore better possibilities ;-)
Btw for the Travis CI a prebuilt binary is used for superior speed and to avoid troubles when code.dlang.org is down.

[andralex]

Yah a makefile rule would be a nice next step.

[ghost]

It was a bad idea. The new pull requests done in a fine style have already begun to fail because they are affected by the old content in the files, that was never checked.

You should rather

• run dscanner before
• run dscanner after
• verify if the diff is empty.

[CyberShadow]

It was a bad idea. The new pull requests done in a fine style have already begun to fail because they are affected by the old content in the files, that was never checked.

No, that's not correct. Phobos was green before the checking was enabled. What happened was that someone merged a PR that was failing on Travis, which cause master to fail. The fix is already on auto-merge.

[wilzbach]

It was a bad idea. The new pull requests done in a fine style have already begun to fail because they are affected by the old content in the files,

It is a test and we are experimenting with it and Travis doesn't affect autotester at all.
Btw do you get emails from Travis? If so, we should enable this during the testing stage.

[ghost]

No i didn't get any mail from travis-CI, anyway, another problem is that some tests can include invalid code or quite unidiomatic code on purpose, which Dscanner could interpret as a problem. I have no concret example just now but I think this is something which can be encountered.

[CyberShadow]

No i didn't get any mail from travis-CI, anyway, another problem is that some tests can include invalid code or quite unidiomatic code on purpose, which Dscanner could interpret as a problem. I have no concret example just now but I think this is something which can be encountered.

Sure. If that ever happens, then that PR can just disable the .dscanner.ini rule in the same PR.

[wilzbach]

The fix is already on auto-merge.

It is merged now, so just rebase and all lights should be green again.
Btw here's another proof of linting in work at Phobos ;-)

[MartinNowak]

Can we please move this into the makefile? There is already checkwhitespace target.
It's fairly inconvenient to have something only in the .travis.yml where you need copy+paste to run it.

[JackStouffer]

If this was relegated to the make file it never would have happened, as no one wants to touch it for fear of breaking it.