After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 571486 - array type syntax doesn't support '?', unowned...
array type syntax doesn't support '?', unowned...
Status: RESOLVED FIXED
Product: vala
Classification: Core
Component: Arrays
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Vala maintainers
Vala maintainers
: 605901 624709 (view as bug list)
Depends on:
Blocks: 620419
 
 
Reported: 2009-02-12 15:17 UTC by Allison Karlitskaya (desrt)
Modified: 2014-05-17 19:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
types: add support for inner type modifiers in arrays (17.06 KB, patch)
2010-02-12 23:17 UTC, Marc-Andre Lureau
reviewed Details | Review
parser: skip_type accept inner parenthesis (1.16 KB, patch)
2010-03-21 22:59 UTC, Marc-Andre Lureau
reviewed Details | Review
parser: parse_type parse inner array type (3.70 KB, patch)
2010-03-21 22:59 UTC, Marc-Andre Lureau
needs-work Details | Review
parser: parse array creation expression with inner type (4.54 KB, patch)
2010-03-21 22:59 UTC, Marc-Andre Lureau
reviewed Details | Review
[RFC] writer: dump inner array type (2.83 KB, patch)
2010-03-21 22:59 UTC, Marc-Andre Lureau
needs-work Details | Review
parser: skip_type accept inner parenthesis (1.16 KB, patch)
2010-03-26 01:44 UTC, Marc-Andre Lureau
reviewed Details | Review
parser: parse_type parse inner array type (3.54 KB, patch)
2010-03-26 01:45 UTC, Marc-Andre Lureau
needs-work Details | Review
parser: parse array creation expression with inner type (4.48 KB, patch)
2010-03-26 01:45 UTC, Marc-Andre Lureau
none Details | Review
[RFC] writer: dump inner array type (2.40 KB, patch)
2010-03-26 01:45 UTC, Marc-Andre Lureau
none Details | Review
parser: accept nullable syntax on array inner type (1.22 KB, patch)
2010-03-26 01:45 UTC, Marc-Andre Lureau
none Details | Review
parser: parse_type parse inner array type (3.04 KB, patch)
2010-09-09 12:59 UTC, Jürg Billeter
reviewed Details | Review
patch to fix nullability of array elements (1.51 KB, patch)
2011-12-09 22:28 UTC, Aaron Andersen
committed Details | Review

Description Allison Karlitskaya (desrt) 2009-02-12 15:17:55 UTC
vala flags this as a syntax error:

error: syntax error, expected ( or [
  string?[] array = new string?[40];
                              ^
Comment 1 Marc-Andre Lureau 2009-11-26 23:09:48 UTC
same with other modifiers such as "unowned"

this is my workaround (still meed to be fully tested though):

public T[] array<T>(int size) { return new T[size] }
Comment 2 Marc-Andre Lureau 2010-01-30 16:50:51 UTC
*** Bug 605901 has been marked as a duplicate of this bug. ***
Comment 3 Marc-Andre Lureau 2010-01-30 17:02:32 UTC
before I start modifying vala, I would like to discuss the syntax change.

Currently unowned T?<>[] is an unowned array of [nullable T<>]. Unowned (& dynamic) apply to the array, not to the inner type. Also, there is no way to specify whether the array is nullable, or pointer of an array.

I propose adding optional parenthesis around inner type so that:

unowned (dynamic T*<>)[]?

Would be a an unowned nullable array of [pointer to dynamic T<>].

What do you think.

obviously, the current code, without parenthesis, should keep the same meaning, with unowned & dynamic modifying the type of the array itself.
Comment 4 Marc-Andre Lureau 2010-02-01 23:35:45 UTC
(In reply to comment #3)
> before I start modifying vala, I would like to discuss the syntax change.
> 
> Currently unowned T?<>[] is an unowned array of [nullable T<>]. Unowned (&
> dynamic) apply to the array, not to the inner type. Also, there is no way to
> specify whether the array is nullable, or pointer of an array.

My bad, nullable array is supported, and pointer of array is out of scope I guess.
Comment 5 Marc-Andre Lureau 2010-02-12 23:17:29 UTC
Created attachment 153672 [details] [review]
types: add support for inner type modifiers in arrays
Comment 6 Marc-Andre Lureau 2010-02-12 23:20:30 UTC
that patch is more a RFC, it could be split, for readibility
Comment 7 Jürg Billeter 2010-03-20 12:28:04 UTC
I think that the approach makes sense. I'm considering using parenthesis in types for tuples anyway. There shouldn't be any conflicts as tuples with less than 2 elements do not need to be supported.

Splitting the patch would be nice as debugging syntax issues later can be quite complex.
Comment 8 Marc-Andre Lureau 2010-03-21 22:59:28 UTC
Created attachment 156692 [details] [review]
parser: skip_type accept inner parenthesis
Comment 9 Marc-Andre Lureau 2010-03-21 22:59:32 UTC
Created attachment 156693 [details] [review]
parser: parse_type parse inner array type
Comment 10 Marc-Andre Lureau 2010-03-21 22:59:36 UTC
Created attachment 156694 [details] [review]
parser: parse array creation expression with inner type

I recall the conflict concerned mainly casts, it is
solved with the help of is_inner_array_type().

It might be possible to simplify the parse_array_creation_expression:
MemberAccess? member or owned DataType? element_type=null situation, by
using simply parse_type(), I don't know clearly how.
For now, it's either given element_type OR member.
Comment 11 Marc-Andre Lureau 2010-03-21 22:59:40 UTC
Created attachment 156695 [details] [review]
[RFC] writer: dump inner array type

This patch seems to do the job, but perhaps break some old behaviour.

My first attempt was quite different, I can't remember why..

For some reason, all array declarations are printed nullable..
Comment 12 Marc-Andre Lureau 2010-03-22 00:31:42 UTC
Review of attachment 156695 [details] [review]:

ok, this break bootstrap, because vapi file are invalid:

gee.vapi:47.40-47.44: error: syntax error, expected `)'
		public abstract G? @get (unowned int index);
Comment 13 Jürg Billeter 2010-03-22 07:33:22 UTC
Review of attachment 156693 [details] [review]:

The patch looks fine (although still untested from my side) except for the following two points:

::: vala/valaparser.vala
@@ +452,3 @@
+			// if this type is not an array, the value is set again a
+			// before returning
+			type.value_owned = true;

I'm missing the reset of this property to false as described in the comment. However, I'd probably just use the following instead:

if (current () == TokenType.OPEN_BRACKET) {
	type.value_owned = true;
}

@@ +464,3 @@
+			if (inner) {
+				Report.error (get_last_src (), "Inner array type not supported.");
+			}

I fail to see `inner' set to true at any point. I'm also not sure why you want to report an error on inner arrays here. Is there a syntactical issue or did you have the code generation issues of stacked arrays in mind?
Comment 14 Jürg Billeter 2010-03-22 07:34:43 UTC
Review of attachment 156692 [details] [review]:

This patch looks fine, I will apply it when at least the second patch is ready as well.
Comment 15 Jürg Billeter 2010-03-22 07:40:30 UTC
(In reply to comment #10)
> Created an attachment (id=156694) [details] [review]
> parser: parse array creation expression with inner type
> 
> I recall the conflict concerned mainly casts, it is
> solved with the help of is_inner_array_type().

I don't see why there could be conflicts with casts after `new'. Can you elaborate? I expect it to work if you just use if (accept (TokenType.OPEN_PARENS)) instead, however, I haven't tested this and might be missing something.

> It might be possible to simplify the parse_array_creation_expression:
> MemberAccess? member or owned DataType? element_type=null situation, by
> using simply parse_type(), I don't know clearly how.
> For now, it's either given element_type OR member.

Why don't you just move UnresolvedType.new_from_expression (member) to the parse_object_or_array_creation_expression method and drop the member parameter?
Comment 16 Jürg Billeter 2010-03-22 07:44:13 UTC
(In reply to comment #12)
> Review of attachment 156695 [details] [review]:
> 
> ok, this break bootstrap, because vapi file are invalid:
> 
> gee.vapi:47.40-47.44: error: syntax error, expected `)'
>         public abstract G? @get (unowned int index);

We should try to avoid unnecessary unowned modifiers and parenthesis in the code writer.

Also, one point that is still missing is the case of the original bug report. That should be allowed even without parenthesis as there is no conflict potential, afaict.
Comment 17 Marc-Andre Lureau 2010-03-22 13:07:36 UTC
(In reply to comment #13)
> Review of attachment 156693 [details] [review]:
> 
> The patch looks fine (although still untested from my side) except for the
> following two points:
> 
> ::: vala/valaparser.vala
> @@ +452,3 @@
> +            // if this type is not an array, the value is set again a
> +            // before returning
> +            type.value_owned = true;
> 
> I'm missing the reset of this property to false as described in the comment.
> However, I'd probably just use the following instead:
> 
> if (current () == TokenType.OPEN_BRACKET) {
>     type.value_owned = true;
> }

oh yeah, good suggestion, done

> 
> @@ +464,3 @@
> +            if (inner) {
> +                Report.error (get_last_src (), "Inner array type not
> supported.");
> +            }
> 
> I fail to see `inner' set to true at any point. I'm also not sure why you want
> to report an error on inner arrays here. Is there a syntactical issue or did
> you have the code generation issues of stacked arrays in mind?

inner is set to true when calling parse_type from inside parse_type. I think I had code generation issues in mind, because it would parse it correctly and generate code, but not with correct ownership. that way it's more clear. At least the intent of this first patch is not to solve every use case at once.
Comment 18 Jürg Billeter 2010-03-22 13:27:51 UTC
(In reply to comment #17)
> (In reply to comment #13)
> > Review of attachment 156693 [details] [review] [details]:
> > @@ +464,3 @@
> > +            if (inner) {
> > +                Report.error (get_last_src (), "Inner array type not
> > supported.");
> > +            }
> > 
> > I fail to see `inner' set to true at any point. I'm also not sure why you want
> > to report an error on inner arrays here. Is there a syntactical issue or did
> > you have the code generation issues of stacked arrays in mind?
> 
> inner is set to true when calling parse_type from inside parse_type. I think I
> had code generation issues in mind, because it would parse it correctly and
> generate code, but not with correct ownership. that way it's more clear. At
> least the intent of this first patch is not to solve every use case at once.

I don't think there would be more issues than what we already have without parenthesis. In some limited cases, arrays of string arrays are already working, so we can't really disallow arrays of arrays in general. If there are cases that are always broken, it certainly makes sense to report an error, however, this should preferably be done in the code generator if it's a backend limitation.
Comment 19 Marc-Andre Lureau 2010-03-22 20:54:30 UTC
(In reply to comment #15)
> > It might be possible to simplify the parse_array_creation_expression:
> > MemberAccess? member or owned DataType? element_type=null situation, by
> > using simply parse_type(), I don't know clearly how.
> > For now, it's either given element_type OR member.
> 
> Why don't you just move UnresolvedType.new_from_expression (member) to the
> parse_object_or_array_creation_expression method and drop the member parameter?

Right, stupid me, done.
Comment 20 Marc-Andre Lureau 2010-03-26 01:44:22 UTC
Created attachment 157133 [details] [review]
parser: skip_type accept inner parenthesis
Comment 21 Marc-Andre Lureau 2010-03-26 01:45:08 UTC
Created attachment 157134 [details] [review]
parser: parse_type parse inner array type
Comment 22 Marc-Andre Lureau 2010-03-26 01:45:12 UTC
Created attachment 157135 [details] [review]
parser: parse array creation expression with inner type

The conflict concerned mainly casts, it is
solved with the help of is_inner_array_type().
Comment 23 Marc-Andre Lureau 2010-03-26 01:45:16 UTC
Created attachment 157136 [details] [review]
[RFC] writer: dump inner array type
Comment 24 Marc-Andre Lureau 2010-03-26 01:45:20 UTC
Created attachment 157137 [details] [review]
parser: accept nullable syntax on array inner type
Comment 25 Jürg Billeter 2010-05-04 09:35:56 UTC
Review of attachment 157134 [details] [review]:

::: vala/valaparser.vala
@@ +405,3 @@
 	}
 
+	DataType parse_type (bool owned_by_default, bool can_weak_ref, bool inner = false) throws ParseError {

The `inner` parameter is completely unused. It's never called with anything but the default and the parameter value is also not used.

@@ -474,3 @@
-			// arrays contain strong references by default
-			type.value_owned = true;
-

This changes parsing result of string[][]. Without the change it is an array of owned arrays of owned strings. With the change it is an array of unowned arrays of owned strings.
Comment 26 Marc-Andre Lureau 2010-05-04 09:41:45 UTC
(In reply to comment #25)
> Review of attachment 157134 [details] [review]:

> The `inner` parameter is completely unused. It's never called with anything but
> the default and the parameter value is also not used.
> 
Sorry, that's a leftover from previous work.
Comment 27 Evan Nemerson 2010-07-19 08:14:32 UTC
*** Bug 624709 has been marked as a duplicate of this bug. ***
Comment 28 Jürg Billeter 2010-09-09 12:59:05 UTC
Created attachment 169850 [details] [review]
parser: parse_type parse inner array type

I've fixed and updated the parse_type patch.
Comment 29 Jürg Billeter 2010-09-09 13:21:01 UTC
Review of attachment 157135 [details] [review]:

This does break some previously valid code. I'm not sure whether it would be a big issue in practice, however, it's bad that it's hard to work around. The usual solution for syntax issues is adding parentheses and that doesn't help in this case.

void main () {
	int[] i = new int[42];
	int j = (((int*) i)[4] + 5);
}
Comment 30 Aaron Andersen 2011-12-09 22:28:35 UTC
Created attachment 203172 [details] [review]
patch to fix nullability of array elements
Comment 31 Aaron Andersen 2011-12-09 22:29:26 UTC
The original problem in this thread still exists in Vala 0.14.1:

> error: syntax error, expected ( or [
>   string?[] array = new string?[40];
>                               ^

I have attached a patch which allows the elements of arrays to be marked as nullable in the initializer.

The patch also reports an error if the nullability of the array elements in the variable and initializer differ:

error: Assignment: Cannot convert from `string[]' to `string?[]?'
string?[] array = new string[40];
          ^^^^^^^^^^^^^^^^^^^^^^

error: Assignment: Cannot convert from `string?[]' to `string[]?'
string[] array = new string?[40];
         ^^^^^^^^^^^^^^^^^^^^^^^
Comment 32 Luca Bruno 2011-12-10 19:11:49 UTC
Review of attachment 203172 [details] [review]:

commit bce3e8474a8238ff5a0bb697a0f94a54c2d959d3
Author: Aaron Andersen <aaron@fosslib.net>
Date:   Sat Dec 10 19:58:18 2011 +0100

    Support creation of arrays with nullable elements
    
    Partially fixes bug 571486.
Comment 33 Luca Bruno 2014-05-17 19:20:48 UTC
commit 0ffe72386589ad6a7f52f95a68929277a31cf719
Author: Luca Bruno <lucabru@src.gnome.org>
Date:   Sat May 17 19:34:06 2014 +0200

    Support (unowned type)[] syntax for transfer-container arrays
    
    Fixes bug 571486

So I've opted for a simpler version based on the above patches. We do not allow any type inside parens, rather only if it begins with (unowned ...), so we avoid ambiguities.
After several tests, the codegen appeared to generate correct code. I've also run a test on several vala projects to see if it broke anything.
If other fixes are needed we can of course work on them on new bug reports.
Feel free to reopen the bug or revert. It's not an absolute decision, but since the syntax has been decided, I preferred to commit something working that can be refined later in case.

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.