GNOME Bugzilla – Bug 596861
Invalid C code when declaring multiple variables of the same name in async methods
Last modified: 2011-11-20 10:17:43 UTC
async string[] func() { return new string[] { "yo", "kurt" }; } async void func2() { if (true) { var response = yield func(); } var response = yield func(); } void main() { } ====================================================== mickey@opal:/local/pkg/vala/test$ LANG=C valac --pkg gio-2.0 doubledeclaration.vala doubledeclaration.vala:6.1-6.16: warning: method `func2' never used async void func2() ^^^^^^^^^^^^^^^^ doubledeclaration.vala:10.7-10.29: warning: local variable `response' declared but never used var response = yield func(); ^^^^^^^^^^^^^^^^^^^^^^^ doubledeclaration.vala:12.6-12.28: warning: local variable `response' declared but never used var response = yield func(); ^^^^^^^^^^^^^^^^^^^^^^^ /local/pkg/vala/test/doubledeclaration.vala.c:33: error: duplicate member 'response' /local/pkg/vala/test/doubledeclaration.vala.c:36: error: duplicate member 'response_size' /local/pkg/vala/test/doubledeclaration.vala.c:37: error: duplicate member 'response_length1' error: cc exited with status 256 Compilation failed: 1 error(s), 3 warning(s)
Confirming. async methods have problems with local temp. variables. Lowering priority as there is an easy workaround though.
*** Bug 606725 has been marked as a duplicate of this bug. ***
*** Bug 606783 has been marked as a duplicate of this bug. ***
It's not that trivial. The workaround would be to use a map to detect collisions when declaring local variables. The problem comes with delegates and related temp variables and their initialization. It's not that easy and requires much testing. Another solution would be to mangle the name when creating the first tree, so the problem would be solved by the parser. But I think you're against it? :)
Created attachment 151751 [details] [review] Detect name collision in coroutine methods This works until: - temp_vars are generated by the ccode visitor (makes _local and _tmp check sane) - hiding outer local variables is forbidden (i.e. var a; { var a; }) Looks like this is not working when an async local variable is captured in a lambda expression (maybe some get_variable_cname is missing?)
I think the above solution is somewhat skipping the syntax work. Therefore, I'd suggest to mangle names right when working with scopes (parser? semantic? I don't know). It would be expected for a coroutine to mangle names because it flatten the scope. Otherwise I was thinking about a solution using inner structs representing scopes: struct Data { struct local_scope { struct inner_scope { gint var; gint var; } } But I wouldn't have any clue on how to implement that :)
Isn't it possible to simply build a variable list first, remove duplicates and build the struct afterwards?
About the inner structs you are proposing - that looks like an ugly hack and/or sneaky attempt to add support for hiding outer scope variables.
(In reply to comment #7) > Isn't it possible to simply build a variable list first, remove duplicates and > build the struct afterwards? No, the variable types might not match.
Ah, right. I keep forgetting that. Sorry about the noise.
Unions?
{ int a = 5; } string a = "hello"; ~~> union { char* string_value; int int_value; } a;
Well your union solution is just like mine, but you add strange mangling and unions instead of structs and still depend on the syntax as I said above. Also inner structs are not a way to support hiding outer variables, I don't know why are you talking about that. So please, care about the bug not what I attempt to do (which I didn't even think about that). The problem is: in C code you have C scopes so you didn't care of it until now. In a struct they're flattened, so you _necessarily need_ to unflatten them in order keep syntax and codegen concepts more separated. I don't propose a fully inner structs solution, it would be overkill, I propose to create one-level inner structs named with a counter: scope1, scope2, ... This requires this change: - refactor get_variable_cexpression (string name) to be get_variable_cexpression (string name, Scope? scope) - a coroutine_scope_map<Scope,CCodeStruct>
Created attachment 151827 [details] [review] inner structs This patch creates inner structs inside the coroutine data block. Also it adds _scope%d_ variables in the cblock so that access is faster. After this, get_variable_cexpression must be refactored to accept a Block, where the block is the block supposing to hold the variable whose cexpression is requested.
The first attachement no longer applies to Vala master, could you please rebase this, so we have a chance to get it in? Thanks a lot!
(In reply to comment #15) > The first attachement no longer applies to Vala master, could you please rebase > this, so we have a chance to get it in? Thanks a lot! It's not a full patch, it's only a proof of having inner structs per block but does not fix the problem. Just to have an ack of type "yes this can be done this way" before actually digging into full patch writing.
*** Bug 645788 has been marked as a duplicate of this bug. ***
Created attachment 185726 [details] [review] codegen: Avoid name clashes in the closure struct Partially fixes bug 596861. This patch partially fix the problem, and it's almost sure that it wouldn't produce any regression. This is because the first variable is always named with the original name, while other names producing clashes will be mangled. So existing code will just work. Non fixed code are try/catch and foreach due to: 1) CatchClause does not accept error_variable as its child 2) ForeachStatement hardcodes variable_name too much instead of using iterator_variable (which is defined but completely unused). So if the above two are real issues, I think the patch correctly addresses the problem.
The patch looks fine to me, seems quite safe. It would make sense to use the regular local variable code path for error and iterator variables as well, assuming it can be done without a significant increase in code complexity.
Created attachment 186497 [details] [review] codegen: Use CatchClause.error_variable over variable_name Notice visit_local_variable will be hopefully replaced with create_local in the future.
Created attachment 187380 [details] [review] codegen: Avoid name clashes in the closure struct Fixes bug 596861.
Created attachment 187381 [details] [review] codegen: Avoid name clashes in the closure struct Fixes bug 596861. Added regression test case.
*** Bug 660652 has been marked as a duplicate of this bug. ***
commit 335636be07c27950791179dc77354d2916c9468a Author: Luca Bruno <lucabru@src.gnome.org> Date: Mon Apr 11 18:08:23 2011 +0200 codegen: Avoid name clashes in the closure struct Fixes bug 596861. 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.
*** Bug 664392 has been marked as a duplicate of this bug. ***