Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable experimental non-null #181

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Enable experimental non-null #181

wants to merge 16 commits into from

Conversation

Bob131
Copy link
Member

@Bob131 Bob131 commented May 30, 2016

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

@Bob131 Bob131 added this to the 0.3.0 milestone May 30, 2016
@arteymix
Copy link
Member

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
Copy link

codecov-io commented May 31, 2016

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
Copy link
Member Author

Bob131 commented May 31, 2016

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
Copy link
Member Author

Bob131 commented May 31, 2016

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

Bob131 added 12 commits May 31, 2016 18:06
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
Copy link
Member Author

Bob131 commented May 31, 2016

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
Copy link
Member

arteymix commented Jun 1, 2016

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

@@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I rebase from master then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

Bob131 commented Jun 1, 2016

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

arteymix commented Jun 1, 2016

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
Copy link
Member Author

Bob131 commented Jun 2, 2016

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
Copy link
Member

arteymix commented Jun 2, 2016

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.

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] == '/')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to get rid of var here?

@arteymix
Copy link
Member

arteymix commented Jun 3, 2016

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
Copy link
Member Author

Bob131 commented Jun 3, 2016

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 Bob131 removed this from the 0.3.0 milestone Jun 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build Valum with --enable-experimental-non-null
3 participants