Elixir: Revisit warnings to be errors

[josevalim]

In this issue, we will keep a list of warnings that we would like to make errors for Elixir v2.0. For now, we will try to have a single thread that lists all of them, but if we cannot manage the discussion, we can break it apart.

It is worth reiterating our compromise with warnings: many places we could error we raise warnings so users can get more feedback on each compilation and we don't allow warnings to be disabled. On the other hand, we promise that every warning will be worth of your time.

The downside of this approach, however, is that folks not always pay attention to warnings and the "getting more feedback on each compilation" is less relevant today than it was 2 years ago since Mix is quite smart when it comes to compiling your code.

Note we won't mention warnings that are raised because of deprecated features. Those, by definition, will raise on 2.0.

Will raise

• [ ] "undefined module attribute @var" should raise - the default behaviour of nil can lead to cascading errors down the road and there is no situation the software will be correct with a missing attribute. By converting it to an error, we can also add a "did you mean?" feature.

Will continue as warning

[josevalim]

I would like to discuss what we will do with:

variable "foo" does not exist and is being expanded to "foo()", please use parentheses to remove the ambiguity or change the variable name

If the function call or the variable also does not exist, an error will be raised anyway. Given parentheses are optional everywhere else, an error may be too imposing, is a warning the correct decision here? That's my inclination.

the variable "foo" is unsafe as it has been set inside a case/cond/receive/if/&&/||

Today this is a warning and the value set inside the case/cond/receive/if/&&/|| leaks out. On 2.0, we are going to change this behaviour so the variable no longer leaks. We still need to decide what we want to do for those cases:

1. warn
2. error
3. do nothing

One thing to have in mind is that folks will write code like this:

x = 1
if true do
  x = 2
end
x #=> 1

and it may be surprising if we don't warn or error. On the other hand, the language was designed to work as above, so why warn/error? Furthermore, we can already write code like this:

x = 1
(fn ->
  x = 2
end).()
x #=> 1

and the code above _does not warn nor error_. If we want to make it a warning or error, it would be nice to do so consistently throughout the language.

[michalmuskala]

Before talking about the substance of the warnings/errors I would like to address the way they are reported. Right now compiler errors are reported right away and stop the compilation. I believe it would make sense for things like that to be reported, but not stop the compilation (allowing other issues to be discovered). It would, though, fail the compilation at the end, so a recompile is required to remedy the problem.

[josevalim]

@michalmuskala I honestly don't think that's possible. An error usually means the module cannot compile and trying to continue will lead to a series of cascading errors that will obfuscate the original error. For example, if a function fails to compile and we don't include it, another function that calls that local function will now fail. If module A depends on moduIe B at compile time and B fails to compile, A will now fail with the wrong reason.

In order to make the compiler proceed in face errors without leading to further errors down the road, we would need to make the compiler aware of _each_ error and how it further affects compilation if we don't want obfuscating errors. That's one of the reasons we keep some warnings as warnings, because the compiler can deduce some behaviour that is reasonable enough for continuing compilation.

We can see such cases in the Erlang compiler. It is easier to recover from errors there, because defining a module in Erlang requires a strict shape, and it is not executable code as in Elixir, but even though failure in defining a function leads to a series of errors and you need to track the original error down (usually at the top). The dynamic nature of Elixir makes this harder.

And even you think "that's worth the effort anyway" you can lead into situations where things such as Code.ensure_compiled?(B) invoke from A will now lead to a different result because of an error. So an error is indeed the best choice.

[michalmuskala]

You are completely right, and I'm not talking about all the compiler errors. But here we're talking about converting things that are workings right now (so further compilation is perfectly possible) to errors, where the compilation would be halted. My comment was addressing those kinds of errors exclusively. This would require the compiler to be aware of the error type. I think this could be possible by having three levels of "errors" in the compiler - critical errors that halt the compilation (corresponding to current errors), regular errors, that would allow the compilation to continue (for former warnings errors) and finally warnings that are reported, but do not affect compilation.

[josevalim]

You are completely right, and I'm not talking about all the compiler errors. But here we're talking about converting things that are workings right now

So they shouldn't be converted to errors. That's it. I am :-1: for introducing a third mode. An error should halt the compilation. That's what an error means. If you believe converting a warning to error will hinder your workflow, then they should not be made errors.

[michalmuskala]

So let's evaluate the change of the warning "undefined module attribute @var" to an error and see how it changes a workflow. Let's assume a project where two such errors were introduced.

Currently:

• I compile the project
• Notice two warnings
• Fix the warnings
• Recompile the project

After the changes:

• I compile the project
• Notice compilation error
• Fix the error
• Recompile the project
• Notice another error
• Fix the error
• Recompile the project

Notice, how we have introduced additional compile-analyse error-fix cycle. On bigger projects, where we need to recompile multiple files, this might be very annoying. A failed compilation requires recompiling again all the files that we were trying to compile, but failed, even if the file was successfully compiled before the error occurred. When recompiling 20-30 files (which is not uncommon for bigger projects) it can take a significant amount of time to fix those errors and recompile the project many times. And here we're only talking about a single type of error. It is my experience than when I make typos, etc, there are usually many related ones throughout the file and multiple compilations are required for this anyway.

To sum it up, I don't think changing those warnings to errors that halt the compilation is a solution that improves the experience. Quite the opposite.

I see couple solutions to this: one is the proposed, non-critical compilation errors that allow the compiler to continue on other files. Another one would be saving the "so far" compiled files on failed compilation, limiting what is recompiled on following attempts.

[ericmj]

There's an easy solution to the workflow problem. If you get errors when switching to Elixir 2.0, switch back to Elixir 1.9 (or whatever) fix your warnings and then switch to Elixir 2.0 when you can compile with zero warnings.

[josevalim]

@michalmuskala the issue though is that a module attribute mismatch can make that same module fail to compile (or another module that is calling a function that relies on it), so it suffers from the cascading error. Although I think this generally indicates we should be conservative on which warnings we are converting to errors.

@ericmj we are not talking about the conversion to 2.0 error but every day development on 2.0.

[ericmj]

we are not talking about the conversion to 2.0 error but every day development on 2.0.

Should we split that out into a third issue :)? The issue seems to talk about warnings that will be converted to errors in 2.0.

[josevalim]

@ericmj no, it is related to this issue. We are discussing if it is worth converting a given warning to an error, given converting it to an error means the compilation will halt.

[ericmj]

Then I think my workflow suggestion above is (partly) an appropriate solution. If we are talking about warnings that have existed for a number of minor versions I think some of them they are fine to convert to errors. You should not bump from Elixir 1.0 to 2.0 without expecting breakage that will disrupt your workflow. If you want a nicer upgrade experience you should run through 1.1, 1.2, 1.3, etc. before.

Converting warnings to errors can definitely be done for some types of warnings. Examples of these are using deprecated functions. There's no reason for someone in Elixir 2.0 to use a function deprecated in 1.2 since the function is not documented anymore and we have been warning about for a number of minor versions. Now there's also the case where we have warnings/errors for common mistakes. Examples of these are zero-arity function calls without parenthesis and variable rebinding in nested scopes. They need to be treated differently.

I think they should still be converted errors but they should not halt compilation. This will require changes to the compiler but continuing compilation for these errors should be safe and should not cause cascading compilation errors since the compiler can continue compilation today when the warning is raised.

[josevalim]

I think they should still be converted errors but they should not halt compilation.

So we should call them something else because they should definitely not be called _errors_. And I believe that a discussion to add such behaviour to the compiler is definitely a separate discussion from this one.

If we find out that providing this feature is a requirement for converting the warnings listed here to errors, then I would close this and come back once we implemented it, but I don't think it is the case.

[josevalim]

Actually, closing this so we can break this discussion further apart.

[OvermindDL1]

Closed I know, but as for your:

I am 👎 for introducing a third mode.

If such a mode were introduced, what if there was a :fatal error, a normal :error, and a :warn. :fatal's stop compilation. :error's continue compilation but still fails the final compile, :warnings are for like deprecation messages and such.

[OvermindDL1]

Or an :error, :soft_error, and :warn...