Mass-adding type restrictions (def foo() : Bar)

[oprypin]

I have developed a semi-automatic workflow to insert type restrictions into Crystal code, and I plan to apply it to the Crystal standard library. The main purpose is to make documentation better. It also guards against some accidental incompatible changes.

The first phase will be only adding method return type restrictions. It's about 1400 annotations added.

• Add return type restrictions: explicitly specify Nil
• Add return type restrictions to number methods / classes
• Add return type restrictions to I/O methods
• Add return type restrictions to JSON, YAML, XML
• Add return type restrictions to methods of container types
• Add return type restrictions to methods related to String
• Add return type restrictions to file-related methods and Log
• Add return type restrictions to crypto-related classes
• Add return type restrictions to HTTP-related classes
• Add return type restrictions to various low-level methods
• Add return type restrictions: uncategorized

Links (I don't guarantee that the links won't break):

• Code (not for merging!!!): oprypin/crystal@argtohtml...annotator~
• Type restrictions so far: https://github.com/oprypin/crystal/compare/annot

The implementation just needs to be fed real usages of the code, and it looks at all the methods that the compiler resolves while compiling the code, and if the method resolves to the same type in every situation, it figures that maybe it's good to add that type as a type restriction. It's super hacky and super unreliable so I filter and fix the results myself.

But I hope that the methods that this automation suggests to restrict are also safe places to make those changes; I'm not coming up with my own ideas, and if I did, I might miss the fact that I'm changing the actual type (e.g. setting it to Nil but it really isn't Nil and someone relied on that).

Semi-related to #4864, mainly because the implementation is based on the patch posted to the issue

[oprypin]

Launched phase 2, adding Nil return type restrictions where the methods currently actually don't return nil but weren't meant to have a return value.

• Change nonsense return types to Nil in IO-related methods
• Change nonsense return types to Nil: JSON and YAML
• Change nonsense return types to Nil in formatter classes
• Change nonsense return types to Nil in HTTP-related methods and Log
• Change nonsense return types to Nil: uncategorized

[asterite]

Keep'em coming! ❤️

[straight-shoota]

I milestoned the entire first batch of PRs. I think they're ready to be merged.
There hasn't been a lot of discussion entries, which I interpret as no general opposition (it could also be that nobody bothered to take a closer look).

The changes are rather big in terms of affected code. But there are presumably no semantic changes.
I'd leave them in the queue for a couple of days to allow for final reviews, but it we wait for too long, conflicts with other PRs may appear again.

[oprypin]

Nice. 5 more conflicts appeared.

[straight-shoota]

The entire first batch has been merged.

[straight-shoota]

About the second batch: These are technically breaking changes because they alter the return value from something that is not Nil to Nil. However, that something was never meant to be returned and is probably of no particularly use. I would consider this a bug fix to avoid unintended leakage of implementation details, which requires altering the return type. So I think it's fine to merge in a minor release, but please speak up if someone thinks this should rather wait for 2.0.

[straight-shoota]

I'd like to test the incoming changes from the second batch with test-ecosystem before merging them to master.
We could do that for each PR individually, or merge them in a combined feature branch first. I suppose the former makes it easier to identify where issues come from.

Should wait after 1.1.1 is released though, to avoid congestion on test-ecosystem CI.

[straight-shoota]

I ran test-ecosystem on all five outstanding PRs and they look good 👍 That doesn't mean it won't break anything else, but it's a encouraging sign. Adding a return type restriction of Nil to methods that were not supposed to return a value in the first place is considerably less likely to cause hidden issues. It would only break code that depends on the (undocumented) return value and we can just say that that's not expected use.

So I think we should go on and merge the Change nonsense return types to Nil PRs.

[sdogruyol]

Gotta love more return types ❤️ Thanks @oprypin @straight-shoota 🙏

[straight-shoota]

The second batch has been merged. Thanks @oprypin

[straight-shoota]

I'm not sure if there's anything left here. Can we close this?

[oprypin]

It could be used for tracking of something in the future, but right now indeed it's best to close it. Thanks