Change unused variables to warnings

[jfecher]

Changes the unused variable error to a warning since they should be optimized out by the ssa pass anyway. This affects all variables, even parameters to main.
This is also noir's first warning instead of a hard error so some small plumbing was added to implement warnings.

[kevaundray]

I think we still want it to be an error and not by default optimised out by the compiler. An unused variable signals an unused constraint, which is most likely a bug.

There can be a flag in the future to ignore it for development mode

[jfecher]

What is the unused constraint in the ssa pass? It was my impression that it only affected the interpreter.

I still think it should be a warning since it is only an efficiency concern. When debugging quick changes fixing these are secondary and slow down the developer. The warning is also annoying enough that it will be fixed eventually still to silence it, thus removing the extra constraint then.

[kevaundray]

In the interpreter it would have been fine to also have unused constraints. The unused constraint is indicative of a bug in the business logic of your code which is why it is a error right now.

Since it's not just an efficiency concern, it would be better if users had to opt out of it explicitly

[jfecher]

I don't see why the unused constraint would be indicative of a bug anymore than an used variable would be indicative of a bug in classical languages. It is true that many times it is - and that is why we still have it as a warning. Many times it is just a mistake left in while debugging though.
For prior art here, Go also errors if there are any unused variables and it is considered a mistake due to the large impact it has on debugging speed.

[kevaundray]

For constraint systems, the biggest source of bugs will be missing constraints, not using a variable is a subclass of this that we should guard against. In classical languages, I think it also makes sense to not release code into production, if there are unused variables.

For golang, I agree that it affects debugging speed, which is why I was suggesting to allow the user to opt out of it when in debug mode

[jfecher]

I think it makes sense not to release code to production unless you already know what it does, have it tested, and know that it does not emit any warnings, or that the warnings it does emit are irrelevant.

I don't think adding a debug switch to this solves this unless it is off rather than on by default since the expected flow would be users try to compile the program, it fails from an unused variable, then they get a bit annoyed and recompile with a -Wunused flag or similar and it works.

I think we are in agreement that unused variables are often indicative of bugs but differ on our stances as to when they must be fixed.

• With an error, all debugging must stop and they must be fixed immediately before anything else (or along with any other errors such as type errors).
• With a warning, the user is free to run/test a nonfinal version of their program before revisiting it later and fixing any warnings, removing or using unused variables.

[guipublic]

I also think you mostly agree, and generating a warning is a simple way to allow the user to opt-out.
We could add a flag that says: --no-warnings and treat all warnings as errors. But this can be done in a separate PR and should not prevent this one to be merged (for the MVP!)

[kevaundray]

Yeah I think we agree for most points too.

The main difference in viewpoint is whether the compiler should automatically opt the user into this behaviour or whether they should not. ie whether the compiler should treat it by default as an error or as an warning.

I think for this particular feature, we should have the user opt out of it/treat them as warnings, since under constrained programs/missing constraints are a common source of bugs. I think it will be fine, if the user gets annoyed and does something like
#[allow(unused_variables)] as long as they are opting themselves into this behaviour.

Regarding releasing code to production, you are a great programmer, so it may come natural to you to make sure that warnings are harmless and everything is tested. In general, I think we cannot assume this though

[kevaundray]

Adding request changes review; details are listed above

[guipublic]

Unsued variables is a source of missing constraint when you manually write your own constraints. With Noir, the compiler writes the constraints for you and it does not forget one. So using Noir is the best way to solve the missing constraint problem. To that regard, I think unsued variables should be treated as they usually are by other programming languages, i.e as a warning.

[jfecher]

Looks like the merge from #479 is now requiring a large, prohibitively expensive merge from master. I'll revert it and see if I can't cherry pick the necessary changes.