Enable experimental non-null #181

Bob131 · 2016-05-30T03:32:21Z

Fixes #37

This patch set just goes through the compiler errors and fixes each of them. I'm yet to add a final commit adding the flag to the meson config file since there's two files that don't seem trivial enough for me to handle on my own: src/valum/valum-router.vala and src/valum/valum-static.vala. An advisory on how to proceed with those would be appreciated.

All the commits are pretty trivial in nature, at worst there's minor changes to control flow. I've tried to document my changes in the commit message bodies as best I can, feedback welcome

arteymix · 2016-05-30T04:46:13Z

Nice work! I'll review the changes when you feel it's ready.

In the meantime, you can add --experimental-non-null to test it on the CI. Hope you're not too confused with the new build system ;)

codecov-io · 2016-05-31T06:58:05Z

Current coverage is 68.25%

Merging #181 into master will decrease coverage by 6.00%

@@             master       #181   diff @@
==========================================
  Files            24         24          
  Lines           765        778    +13   
  Methods           0          0          
  Messages          0          0          
  Branches         89         91     +2   
==========================================
- Hits            568        531    -37   
- Misses          197        221    +24   
- Partials          0         26    +26

Powered by Codecov. Last updated by 9c68c05...2d55149

Bob131 · 2016-05-31T07:04:37Z

Er, okay. So it appears as though the ancient vapis used in the Travis builder bind libsoup with some incorrect function sigs. I'm not sure what to do about that. For my own projects I'd normally just ship a patched vapi; is that appropriate here?

Non-obvious changes: * Line 131: valac chokes on using different types for the comparison to the return with the ternary operator. No idea why. * Line 156: Looking at https://github.com/GNOME/libsoup/blob/master/libsoup/soup-headers.c#L462 it doesn't seem as though header_parse_list() will push a null value onto the SList, so remove the null check. Instead, test whether Soup.Cookie.parse returned a valid result.

* Changed line 70 in line with previous commit * Type cast on line 281 to get around incorrect function signatures on old VAPIs

Non-obvious changes: * Line 73: Using the `new` operator to init the OptionEntry array nets us a null-terminated array, avoiding issues with the OptionEntry binding

This commit edits the OptionEntry setup at line 50-ish, in line with the previous commit.

Bob131 · 2016-05-31T07:36:35Z

Oh, nvm. Sorry about that, I hadn't actually thought of using type coercion to fix that. I'll get back to you soon re extra changes

Fiddle with OptionEntry-related lines from 200 onwards to get the compile working, as in previous commits. Unlike previous commits, however, this screws up the option order to fit into the `if soup` macros. Otherwise, just the usual syntax amendments.

Nothing special to report in this commit, just going through and correcting minor compiler errors.

Since DataInputStream.read_upto() returns null only as an error mode, replace null checks with try..catch blocks to avoid having to mess with GLib's vapi.

Fix up OptionEntry definitions, else just minor stuff as usual.

Exactly what it says on the tin, nothing unusual to report.

The only change to this file is line 60, where now we test to ensure the regex capture we're after is non-null.

Just marking the types hashmap as non-null after the null check.

Just marking printf args as non-null around line 105.

Coerce location string types to `string?` to avoid incorrect function signatures on old libsoup VAPIs and mark them non-null after successful null check.

* Cast MessageHeaders.get_list() result to `string?` to avoid issues with old libsoup VAPIs * Rejig for loop to use a nullable reference to the SList

Just various fixes re faulty libsoup bindings, mostly explicitly marking types as nullable.

Adds `--enable-experimental-non-null` to meson.build.

Bob131 · 2016-05-31T08:18:18Z

Alright, there we go. So that build fails (starting at https://travis-ci.org/valum-framework/valum/builds/134095170#L748) because I haven't touched valum-router.vala and valum-static.vala. I've never looked at (or even knew about) runtime flag introspection and boolean operators are still kinda new to me, so that's why I've ignored the former. As for the latter, I'm not familiar enough with Valum to know what's really going on, eg I don't know whether certain cxt members are guaranteed not to be null or whether it's a failure mode, or how failures should be handled in that closure.

Besides those two files remaining problematic, the rest of the patch set is good for review (or at least I think so, hopefully I didn't forget anything ;)

arteymix · 2016-06-01T02:26:59Z

Is it necessary to cast expression like nullable ?? default? If so, Vala need some type inference work.

arteymix · 2016-06-01T03:18:11Z

src/valum/valum-content-negotiation.vala

@@ -92,9 +92,9 @@ namespace Valum.ContentNegotiation {

 			string? best_expectation = null;
 			double  best_qvalue      = 0;
-			foreach (var accepted in Soup.header_parse_quality_list (header, null)) {
+			foreach (var accepted in Soup.header_parse_quality_list ((!) header, null)) {


I changed this (added a if branch`) in d463968 to support unacceptable expectations.

Should I rebase from master then?

Could help, but it's easy to merge. You should squash all the fixes in a single commit. This can go on forever ;)

Would you prefer to merge a single commit? I went with a bunch of smaller commits so that the commit messages could accurately describe the changes; I thought it might make the review process a bit easier. But I'm certainly not averse to squashing

Multiple commits is fine for reviewing; I'll squash on the merge.

It's almost always a matter of history readability and ability to revert changes efficiently. If all the fixes are spread among dozens of commits, it makes both criteria hard to satisfy.

Bob131 · 2016-06-01T04:39:17Z

Unfortunately it is necessary to cast such expressions. It's a bit of an eyesore, but I find syntactical inconveniences like that to be well worth the added safety. Not that your code seems to have suffered too much!

It may well be a bug in valac which I might investigate at some point, but my experience of the Vala project on GNOME's Bugzilla has been pretty lacklustre.

arteymix · 2016-06-01T18:28:07Z

src/vsgi-scgi/vsgi-scgi.vala

-			if (size_str == null) {
+			try {
+				size_str = reader.read_upto (":", 1, out length);
+			} catch {


It seems like read_upto is not correctly annotated because it actually return null without raising an error.

Roger, will fix, but that seems like an upstream bug. Do you have a source file link that shows where it returns null without setting error?

It's something I discovered experimentally. It's either a bug or a EOF behaviour.

The docs does not state that null is returned exclusively on error:

a string with the data that was read before encountering any of the stop characters. Set length to a gsize to get the length of the string. This function will return NULL on an error.

In any case, the annotation should be fixed in the VAPI.

arteymix · 2016-06-01T23:15:36Z

Casting to non-null is not a safer approach and may actually hide bugs. Maybe you should look at the possibility of introducing explicitly typed variables? Something like..

string? nullable_a = null;
string a = nullable_a ?? "defaut";

In the past, I attempted to work on this, but I resigned because it introduced bugs. It seems like the (!) operator is not idempotent.

Bob131 · 2016-06-02T01:31:32Z

What I was trying to say was that the second example doesn't work, you must cast an expression with the ?? operator to non-null (as little sense as that makes). Of course the (!) cast can be dangerous and discretion must be exercised with its use; if you notice a patch where I haven't taken due care, please let me know. Otherwise, I'm not sure what you're talking about.

arteymix · 2016-06-02T15:28:23Z

It was more a general idea to avoid the (!) operator by introducing an explicitly typed variable. Likewise I think that it would be important to use it only of necessary because it hides potential bug.

arteymix · 2016-06-02T15:29:31Z

src/valum/valum-basepath.vala

-						var location = res.headers.get_one ("Location");
-						if (!res.head_written && location != null && location[0] == '/')
+						string? location = res.headers.get_one ("Location");
+						if (!res.head_written && location != null && ((!) location)[0] == '/')


Is it necessary to get rid of var here?

arteymix · 2016-06-03T04:44:49Z

I really think we should postpone that until we get better support for null-checking in the compiler. It wouldn't hurt to apply the --enable-experimental-non-null flag from a specific version of valac.

Bob131 · 2016-06-03T04:56:15Z

Sounds good to me.

On Fri, Jun 3, 2016 at 2:44 PM, Guillaume Poirier-Morency
[email protected] wrote:

I really think we should postpone that until we get better support
for null-checking in the compiler. It wouldn't hurt to apply the
--enable-experimental-non-null flag from a specific version of valac.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.

Bob131 added the enhancement label May 30, 2016

Bob131 added this to the 0.3.0 milestone May 30, 2016

Bob131 added 4 commits May 31, 2016 17:32

Make src/vsgi/vsgi-response.vala non-null compliant

8037fbf

* Changed line 70 in line with previous commit * Type cast on line 281 to get around incorrect function signatures on old VAPIs

Make src/vsgi/vsgi-server.vala non-null compliant

f1070a2

Non-obvious changes: * Line 73: Using the `new` operator to init the OptionEntry array nets us a null-terminated array, avoiding issues with the OptionEntry binding

Make src/vsgi/vsgi-socket-listener-server.vala non-null compliant

c1939a7

This commit edits the OptionEntry setup at line 50-ish, in line with the previous commit.

Bob131 added 12 commits May 31, 2016 18:06

Make src/vsgi-cgi/vsgi-cgi.vala non-null compliant

623bd0f

Nothing special to report in this commit, just going through and correcting minor compiler errors.

Make src/vsgi-scgi/vsgi-scgi.vala non-null compliant

845f528

Since DataInputStream.read_upto() returns null only as an error mode, replace null checks with try..catch blocks to avoid having to mess with GLib's vapi.

Make src/vsgi-fastcgi/vsgi-fastcgi.vala non-null compliant

f1b0ab9

Fix up OptionEntry definitions, else just minor stuff as usual.

Make src/valum/valum-context.vala non-null compliant

22d6796

Exactly what it says on the tin, nothing unusual to report.

Make src/valum/valum-regex-route.vala non-null compliant

153e1e7

The only change to this file is line 60, where now we test to ensure the regex capture we're after is non-null.

Make src/valum/valum-rule-route.vala non-null compliant

5447410

Just marking the types hashmap as non-null after the null check.

Make src/valum/valum-server-sent-events.vala non-null compliant

ea965aa

Just marking printf args as non-null around line 105.

Make src/valum/valum-basepath.vala non-null compliant

fad5671

Coerce location string types to `string?` to avoid incorrect function signatures on old libsoup VAPIs and mark them non-null after successful null check.

Make src/valum/valum-decode.vala non-null compliant

005d40e

* Cast MessageHeaders.get_list() result to `string?` to avoid issues with old libsoup VAPIs * Rejig for loop to use a nullable reference to the SList

Make src/valum/valum-content-negotiation.vala non-null compliant

ca384da

Just various fixes re faulty libsoup bindings, mostly explicitly marking types as nullable.

Enable non-null valac flag

695f3a6

Adds `--enable-experimental-non-null` to meson.build.

arteymix reviewed Jun 1, 2016
View reviewed changes

arteymix assigned Bob131 Jun 1, 2016

arteymix reviewed Jun 2, 2016
View reviewed changes

Bob131 removed this from the 0.3.0 milestone Jun 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable experimental non-null #181

Enable experimental non-null #181

Bob131 commented May 30, 2016

arteymix commented May 30, 2016

codecov-io commented May 31, 2016 •

edited

Loading

Bob131 commented May 31, 2016

Bob131 commented May 31, 2016

Bob131 commented May 31, 2016

arteymix commented Jun 1, 2016

arteymix Jun 1, 2016

Bob131 Jun 1, 2016

arteymix Jun 1, 2016

Bob131 Jun 2, 2016

arteymix Jun 2, 2016

Bob131 commented Jun 1, 2016

arteymix Jun 1, 2016

Bob131 Jun 2, 2016

arteymix Jun 2, 2016

arteymix commented Jun 1, 2016

Bob131 commented Jun 2, 2016

arteymix commented Jun 2, 2016

arteymix Jun 2, 2016

arteymix commented Jun 3, 2016

Bob131 commented Jun 3, 2016

Enable experimental non-null #181

Are you sure you want to change the base?

Enable experimental non-null #181

Conversation

Bob131 commented May 30, 2016

arteymix commented May 30, 2016

codecov-io commented May 31, 2016 • edited Loading

Current coverage is 68.25%

Bob131 commented May 31, 2016

Bob131 commented May 31, 2016

Bob131 commented May 31, 2016

arteymix commented Jun 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bob131 commented Jun 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arteymix commented Jun 1, 2016

Bob131 commented Jun 2, 2016

arteymix commented Jun 2, 2016

Choose a reason for hiding this comment

arteymix commented Jun 3, 2016

Bob131 commented Jun 3, 2016

codecov-io commented May 31, 2016 •

edited

Loading