Pre-SIP: warn against unidiomatic features (e.g., null value and unsafe casts)

There are some features that are currently allowed in Scala, but that are widely considered unidiomatic and are thus strongly discouraged. However, this is mostly a cultural consensus and nothing in the language makes it clear that these features are actually “second class” citizens. Note that these features are often still useful in a restricted and isolated context, such as performance-sensitive code or private type-unsafe implementations of type-safe interfaces, so we don’t want to just remove them.

These features include:

  • the null value (and probably also of the Null type)

  • unsafe casts with asInstanceOf

  • @unchecked in pattern matching

  • @uncheckedVariance in member declarations

More features can probably be added to that list; if you think of one that I’m missing, be sure to leave a comment!

Unless used very carefully, all these features lead to unsoundness (mismatch between the static and dynamic semantics of Scala). For this reason, I’d argue that they should be explicitly discouraged, so that beginners are not tempted to use them. I propose to require language imports in order to explicitly enable them, and to raise warnings if they are used without the imports.

Part of my motivation is that I’ve seen beginners in some places make very liberal use of features like unsafe casts or null (which are totally normal in some languages), not realizing that this is not the way Scala is generally “meant” to be used most of the time.

EDIT: to clarify, one would either import the language features as in:

import scala.language.{nullValues, typeCasts}

…or just use a compiler option like -language:nullValues, exactly like how it’s currently done for things like reflectiveCalls or postfixOps, so it would not be a big impediment for people who think these feature should be allowed project-wide.

6 Likes

I like the idea, with sensible and minimal defaults (certainly null applies, and probably asInstanceOf). Could this perhaps be made configurable and extensible, similar to WartRemover? That way, a powerful linting tool is injected into the compiler as well. I think @smarter mentioned the idea to me over a year ago.

Should certainly add erased types in type patterns; i.e.,

@ (Some("foo"): Any) match { case x: Option[Int] => x } 
res9: Option[Int] = Some("foo")

There are a lot of soundness holes but I think this is probably the biggest one. Every Scala programmer falls into it at some point. Honestly I have no idea why this compiles at all. There is evidently a reason.

4 Likes

Thankfully, your example at least gives a warning: “non-variable type argument Int in type pattern Option[Int] is unchecked since it is eliminated by erasure”. (Note: If you’re using the Ammonite REP you may not see the warnings.)
Removing this warning requires @unchecked, which is already part of my list :^)

2 Likes

I think some of these things are not like the others.

On the one hand, @unchecked and @uncheckedVariance are already explicit declarations that permit something otherwise forbidden. I’m not sure requiring two different explicit flags is better than one. (And if two are better, why not three?)

Arguably, asInstanceOf is in the same category: it allows code to compile that would not do so otherwise, and it has a syntax that’s (perhaps deliberately) more explicit and verbose than the equivalent in other languages. Perhaps we should treat differently cases where the cast can’t be proven sound (which would make it unnecessary), from cases where it can proven unsound; it might make sense to disallow code like 1.asInstanceOf[String].

On the other hand, I don’t understand how the null value leads to unsound code, as opposed to merely un-idiomatic and potentially buggy code. (Please correct me if I’m wrong) Dotty supports null and the Null type but the dot calculus was proven sound. There are very many un-idiomatic things; this is the territory of a liner or style checker, not the compiler. However, requiring an import to use the null literal would mean putting that import into nearly every file that uses the Java stdlib and most other Java libraries, which includes a lot of Scala library code too.

1 Like

Seems like a reasonable approach. I have slightly mixed feelings WRT asInstanceOf, since it is typically necessary when writing Scala.js facades, but an import/warning like this might help encourage people to do the grungy JavaScript interface work in facades rather than in ad-hoc code. And I think it’s a lovely idea for null – I agree that this is one of the bigger traps for beginners…

This is actually a warning which you can turn into an error with scalac flags

I think it is fine to import scala.language.{nullValues, typeCasts} when we write code at the boundaries of the Scala world (ie. JavaScript facades or Java calls). I like the idea of requiring such an import like we currently do with implicitConversions.

7 Likes

When has the barrier of language import stopped anyone from using any of the features? Even though the intention is good, IDE suggestions to import the required language import will make this of little practical value.

I sense you are only considering yourself and your own use cases and projecting that out into the world as the only perspective of evaluating this. Consider these following other perspectives:

  1. It helps gently push a beginner away from non-idiomatic ways of doing things. Granted, it doesn’t automatically teach the proper idiomatic way to do things.
  2. It gives an opportunity for a non-beginner to think explicitly about this activity. I personally find this highly desirable. It has been fantastic for other things like implicit conversions. Granted, it is trivially passed with an auto-import. I still like the fact that I am stopped and must consciously make a choice.
  3. It enables automated tools to quickly locate where these issues likely lie in a codebase (by scanning just for imports) without having to deep parse each and every source file.
5 Likes

I sense you are only considering yourself and your own use cases and projecting that out into the world as the only perspective of evaluating this.

Those "only"s are rather harsh. I don’t think anyone can truly project anything other than one’s experience (and a perception of co-developer’s experience). So I stated based on what I have experienced and what I have seen others have experienced to hopefully serve as one input.

Now on serious technical points:

  1. I work with beginner Scala developers and usually I find that such warnings/errors act as a nuisance, especially since they aren’t equipped with implementing an alternative superior way, so they jump to whatever the IDE suggests and move on. Especially for nulls and typecasts, solutions may not be simple/local (for example may require changing other parts of code that may have handed them an ill-typed object).

  2. Nudge/enforcement offered by language import often ends up (through a quick fix or manually) as imports at the top-level (which IDEs helpfully/unhelpfully fold and hide). Once accepted for a file, it loses value as a fine-grained nudge and (especially for null and typecast kinds of checks, it rarely makes sense as a good/bad thing for the whole source file).

  3. True that I can ‘grep’ for language imports to find a certain unwanted usage, but only to the extent that import language._ isn’t used. I don’t think parsing the whole fine is such a hard problem as already illustrated by wartremover etc.

IDEs and quick fixes/inspections offered by them is today’s reality, so any solution has to consider it existence and how developers interact with these tools.

This is true, but I run into people who are doing it and they ignore the warning because they don’t understand it and hey it’s only a warning. It trips up beginners.

As a person who makes a living writing both Scala and Java code I have to disagree with this proposal. A selling point of Scala is its Java interop, and requiring me to use an import just to pass null or check a method result for null is counter to Java interop and would discourage me from using Scala in the future. If I’m not allowed to write null, I’m going to end up writingNone.orNull, which is in no way better.

I like the proposal, but understand the sentiment. Perhaps a compiler flag to disable all such checks would work for you?

Reminds me of the Go joke “you can use the whole language!”, though Go has nulls too so I have to disagree a bit.

An alternative is to have a pure mode for Scala. If you want to turn off core language features, turn it on, and scalac will disallow null, var, while, and so on.

asInstanceOf, @unchecked and @uncheckedVariance I don’t have much of an opinion on.

What about the current language imports, which already hide core language features behind language imports? Would you propose to remove them?

That’s a separate discussion. I’m specifically against requiring an import for null.

I agree with you.
I just do not understand why null can be an unidiomatic feature.

So there is no alternative to null

var a:String = _  // it is good 
a = null   // is it  really unidiomatic? 

Note that this is similar to asInstanceOf on Scala.js. The question comes down to style, specifically of how you interact with libraries defined in another language.

The proposal, if accepted, would strongly favor a style of “wrapping” the external language. I think the only real difference is that the Scala.js community has, from the beginning, strongly encouraged the creation of Scala facades around JS libraries. Hence, it’s an easier sell to make asInstanceOf require an import, since the usage is already mainly in the facade files, not in business code.

The difference is that we are much less in the habit (as an overall community) of wrapping Java libraries, even when those libraries’ APIs are poorly suited to Scala. This change would nudge us in the direction of writing such wrappers. I actually think that would be a positive change, but YMMV…

3 Likes

Not entirely – I personally think consistency is important in this matter.

Having language imports for features deemed dangerous or advanced is a single topic.