GNOME Bugzilla – Bug 571486
array type syntax doesn't support '?', unowned...
Last modified: 2014-05-17 19:20:48 UTC
vala flags this as a syntax error: error: syntax error, expected ( or [ string?[] array = new string?[40]; ^
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] }
*** Bug 605901 has been marked as a duplicate of this bug. ***
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.
(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.
Created attachment 153672 [details] [review] types: add support for inner type modifiers in arrays
that patch is more a RFC, it could be split, for readibility
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.
Created attachment 156692 [details] [review] parser: skip_type accept inner parenthesis
Created attachment 156693 [details] [review] parser: parse_type parse inner array type
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.
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..
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);
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?
Review of attachment 156692 [details] [review]: This patch looks fine, I will apply it when at least the second patch is ready as well.
(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?
(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.
(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.
(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.
(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.
Created attachment 157133 [details] [review] parser: skip_type accept inner parenthesis
Created attachment 157134 [details] [review] parser: parse_type parse inner array type
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().
Created attachment 157136 [details] [review] [RFC] writer: dump inner array type
Created attachment 157137 [details] [review] parser: accept nullable syntax on array inner type
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.
(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.
*** Bug 624709 has been marked as a duplicate of this bug. ***
Created attachment 169850 [details] [review] parser: parse_type parse inner array type I've fixed and updated the parse_type patch.
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); }
Created attachment 203172 [details] [review] patch to fix nullability of array elements
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]; ^^^^^^^^^^^^^^^^^^^^^^^
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.
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.