(interpreter) Detect reassignment from incoercible values #188

perlun · 2021-04-18T06:01:06Z

A typical example of an incoercible assignment is this:

var i = 1;
i = "foo";

...in other words, an integer value being reassigned a string value.

~~Other use cases are more tricky. For example, should we or should we not allow nil assignment by default for reference types? We'll try to decide on this before merging this PR.~~ We settled on emitting a warning on nil assignment, which is considered an error by default but can be demoted to a warning if necessary.

perlun · 2021-04-18T06:04:24Z

src/Perlang.Tests.Integration/Typing/TypingTests.cs

+            var output = EvalReturningOutputString(source);
+
+            Assert.Equal("nil", output);
+        }


Other use cases are more tricky. For example, should we or should we not allow nil assignment by default for reference types? We'll try to decide on this before merging this PR.

In other words, this. ☝️ We need to figure out whether this is what we want or not. Having it like this is the easiest way out, but it should actually be quite simple to forbid all nil assignments also if we like. We could actually give it a try and see how many tests it breaks. 💡

It's probably much easier to forbid nil assignment at this early stage in the Perlang development than trying to get rid of it after-the-fact, which experience from other languages has proven to be quite hard (not to say impossible). (Well, C# has made it possible to default to non-nullability by default which is quite an achievement.)

The following is now in place:

Reassignments of variables to nil emits a warning.

Using nil as a variable initializer likewise emits a warning

Bug fix: Using nil for local variable inference (var s = nil) is now properly detected as an invalid condition, causing a compilation error. There is simply no way for the compiler to determine what type the user was intending to use in that case, so it's better to just fail fast.

Passing nil as a function parameter also emits a warning.

The PR got a bit more complex because of this. I'll try to extract the infrastructure for emitting warnings into a separate PR in a day or two, since it's an isolated piece of art that doesn't necessarily need to be glued together with the rest like this.

One big caveat at the moment is also the following semantic detail:

In REPL mode, warnings are treated as warnings and just produce some noise; the code will still run as if the warning wasn't there.

In script mode, warnings are treated as errors (imagine -Werror being enabled with gcc) and there is no way to turn it off. The latter might be a bit unpleasant and we could try to fix a -Wno-error-flag or something when we have the infra for warning extracted to an isolated PR.

One big caveat at the moment is also the following semantic detail:

In REPL mode, warnings are treated as warnings and just produce some noise; the code will still run as if the warning wasn't there.

In script mode, warnings are treated as errors (imagine -Werror being enabled with gcc) and there is no way to turn it off. The latter might be a bit unpleasant and we could try to fix a -Wno-error-flag or something when we have the infra for warning extracted to an isolated PR.

This was now fixed. I added a -Wno-error=nil-usage flag that you can use to demote usage of nil to just a warning instead of an error.

The REPL still always treats it as a warning which makes sense in my world view of how a REPL should "feel" to the user.

One caveat still: the EvalHelper methods will consider all warnings warnings by default, not errors. We could change this (and perhaps we should) but it does berak about 5 unit tests. Some of them could easily be fixed but not others. We would probably have to figure out a way to disable/enable warnings for individual tests to accomplish that (perhaps by moving them to be Program integration tests and passing in the proper -Wno-error flag). But, that's a different story for another day.

I'll try to extract the infrastructure for emitting warnings into a separate PR in a day or two, since it's an isolated piece of art that doesn't necessarily need to be glued together with the rest like this.

This part I'll try to deal with before we get this merged though.

This part I'll try to deal with before we get this merged though.

For reference, infrastructure was extracted to #190 and #191, both of which are merged by now.

This is an extract of most of the infrastructure code from #188, to make that PR clearer and more focused on the actual semantic changes.

perlun · 2021-05-12T19:30:39Z

src/Perlang.ConsoleApp/Program.cs

@@ -85,14 +85,32 @@ public static int MainWithCustomConsole(string[] args, IConsole console)
            var detailedVersionOption = new Option("-V", "Show detailed version information");
            var evalOption = new Option<string>("-e", "Executes a single-line script") { AllowMultipleArgumentsPerToken = false, ArgumentHelpName = "script" };
            var printOption = new Option<string>("-p", "Parse a single-line script and output a human-readable version of the AST") { ArgumentHelpName = "script" };
+            var noWarnAsErrorOption = new Option<string>("-Wno-error", "Treats specified warning as a warning instead of an error.") { ArgumentHelpName = "error" };
+
+            var disabledWarningsAsErrorsList = new List<WarningType>();


(Silly List suffix to avoid name clash with non-static field. It's a shame the C# compiler uses the same scope for these, since a static method cannot access the fields anyway. Let's try to do better in Perlang when we implement support for static methods. 😎)

perlun · 2021-05-12T19:30:59Z

src/Perlang.Interpreter/Typing/TypesResolvedValidator.cs

+
+            if (expr.Value.TypeReference.IsNullObject)
+            {
+                // TODO: Use expr.Value.Token here instead of expr.name, #189


perlun · 2021-05-12T19:31:08Z

src/Perlang.Interpreter/Typing/TypesResolvedValidator.cs

@@ -379,6 +384,11 @@ public override VoidObject VisitVarStmt(Stmt.Var stmt)
                            $"Cannot assign {stmt.Initializer.TypeReference.ClrType.Name} value to {stmt.TypeReference.ClrType.Name}"
                        ));
                    }
+                    else if (stmt.Initializer.TypeReference.IsNullObject)
+                    {
+                        // TODO: Use stmt.Initializer.Token here instead of stmt.name, #189


A typical example of an incoercible assignment is this: var i = 1; i = "foo"; ...in other words, an integer value being reassigned a `string` value. Other use cases are more tricky. For example, should we or should we not allow `nil` assignment by default for reference types? We'll try to decide on this before merging this PR.

perlun added the enhancement New feature or request label Apr 18, 2021

perlun added this to the 0.1.0 milestone Apr 18, 2021

perlun commented Apr 18, 2021

View reviewed changes

perlun force-pushed the fix/ensure-variables-are-not-reassigned-with-incoercible-values branch 6 times, most recently from fff1220 to eb43349 Compare May 5, 2021 20:10

perlun force-pushed the fix/ensure-variables-are-not-reassigned-with-incoercible-values branch 2 times, most recently from 54c29ad to 4e19384 Compare May 8, 2021 20:24

This was referenced May 9, 2021

(interpreter) Add infrastructure for compiler warnings #190

Merged

(consoleapp) Improve argument parsing #191

Merged

perlun added a commit that referenced this pull request May 12, 2021

(interpreter) Add infrastructure for compiler warnings (#190)

c1ca18f

This is an extract of most of the infrastructure code from #188, to make that PR clearer and more focused on the actual semantic changes.

perlun force-pushed the fix/ensure-variables-are-not-reassigned-with-incoercible-values branch 2 times, most recently from 5e81fd1 to ed3d19b Compare May 12, 2021 19:29

perlun commented May 12, 2021

View reviewed changes

perlun force-pushed the fix/ensure-variables-are-not-reassigned-with-incoercible-values branch 2 times, most recently from 69c6e30 to 159dfa2 Compare May 22, 2021 05:39

perlun force-pushed the fix/ensure-variables-are-not-reassigned-with-incoercible-values branch from 159dfa2 to 588a089 Compare May 22, 2021 20:12

perlun merged commit 2c83bd9 into master May 22, 2021

perlun deleted the fix/ensure-variables-are-not-reassigned-with-incoercible-values branch May 22, 2021 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(interpreter) Detect reassignment from incoercible values #188

(interpreter) Detect reassignment from incoercible values #188

perlun commented Apr 18, 2021 •

edited

Loading

perlun Apr 18, 2021

perlun Apr 29, 2021

perlun May 8, 2021

perlun May 12, 2021

perlun May 12, 2021

perlun May 12, 2021

perlun May 12, 2021

(interpreter) Detect reassignment from incoercible values #188

(interpreter) Detect reassignment from incoercible values #188

Conversation

perlun commented Apr 18, 2021 • edited Loading

perlun Apr 18, 2021

Choose a reason for hiding this comment

perlun Apr 29, 2021

Choose a reason for hiding this comment

perlun May 8, 2021

Choose a reason for hiding this comment

perlun May 12, 2021

Choose a reason for hiding this comment

perlun May 12, 2021

Choose a reason for hiding this comment

perlun May 12, 2021

Choose a reason for hiding this comment

perlun May 12, 2021

Choose a reason for hiding this comment

perlun commented Apr 18, 2021 •

edited

Loading