Bug 359165

Comment 1 fahua jin 2011-09-27 22:35:25 EDT

Created attachment 204135 [details]
The sample EGL file.

Comment 2 Brian Svihovec 2011-09-28 09:50:29 EDT

While this is most likely something that we need to fix before .7 ships, I do not think that it is blocking green threads or the VE.  Let me know if this is not the case.  

In practice, users should not define a variable with the following syntax:

var1 Button = new Button{text = "foo"};

In this code, two button instances are created, and one is thrown away.  The statement should be:

var1 Button{text = "foo"};

The 'new' statement would be required if var1 was declared as nullable:

var1 Button? = new Button{text= "foo"};

but this is not used by the VE or our samples.

Also, our existing samples define anonymous widgets using the new expression, which seems to be working correctly:

box.children = [new Button{text = "foo"} ] ;

Comment 3 Huo Zhen Zhong 2011-10-08 04:32:57 EDT

I think this must be fixed asap, because it block one major defect 359167 of VE.

Comment 4 Brian Svihovec 2011-10-10 13:59:42 EDT

Lowering the severity and the priority of this defect, due to changes in 359167.

Comment 5 Scott Greer 2011-10-24 10:16:08 EDT

As Brian says, this idiom essentially results in two "new" statements.  

The second new invalidates our current, somewhat limited approach to avoiding the forward references problem that can occur in construction / initialization.  Right now the way this works is that a part will be generated with two functions:  eze$$setEmpty and eze$$setInitial.  The flow is that the constructor calls eze$$setInitial, which in turn calls eze$$setEmpty.   eze$$setEmpty is responsible for all instantiations of reference objects;  the idea is that by instantating everything before we process initializers, we're less dependent on the order in which we execute those initializers.    

However, this 2nd new statement invalidates that assumption because it changes the reference for that field.  Other initializers that reference this field, such as the case for children of a widget, will only work if they are executed after the 2nd "new" occurs.

When we discussed forward references, Tim mentioned the idea of working on some call flow analysis for EGL to help with these kinds of problems.   Something like a reference graph is needed so that we can derive the optimum order in which the fields should be initialized.  This would perhaps belong in the "preprocessor" component we've talked about adding, since this logic belongs between the IRs and the generators -- this issue isn't specific to JS gen.

Comment 6 Scott Greer 2011-10-24 10:22:20 EDT

*** Bug 359169 has been marked as a duplicate of this bug. ***

Comment 7 Brian Svihovec 2011-11-01 13:40:08 EDT

Targeting for 1.0, although I am not sure if we should ever fix this.  Would it make sense to update source for a widget if it has been initialized with a New statement?

Comment 8 fahua jin 2011-11-02 22:18:31 EDT

(In reply to comment #7)
> Targeting for 1.0, although I am not sure if we should ever fix this.  Would it
> make sense to update source for a widget if it has been initialized with a New
> statement?

Jiyong or Xiaobin,

If my understanding for Brian's comment is correct, does it mean that all of your widget source code need to be updated for removing the new statement? Thanks.

Comment 9 Brian Svihovec 2011-11-03 07:33:24 EDT

Regarding comment 8, can you provide specific examples of what you think may have to change?

Comment 10 fahua jin 2011-11-03 22:13:40 EDT

(In reply to comment #9)
> Regarding comment 8, can you provide specific examples of what you think may
> have to change?

I don't know, if we don't have, then just ignore my comments.

Comment 11 Huang Ji Yong 2011-12-15 02:56:02 EST

Hi Brian and Jeff,
I think the generation is right as described in comment 5 by Scott.
In RBD, the setting block of new expression in the initialization part is reorganized in IR.
For example,
handler myhandler{
  a Button = new Button{text = "test"};
}
is reorganized in IR to be like
handler myhandler{
  a Button{text = "test"};
}
Do you think this make sense to EDT?

Comment 12 Brian Svihovec 2012-01-12 17:09:07 EST

I discussed this with Paul Harmon, and we will not update the IR to convert the following:

handler myhandler
  a Button = new Button{text = "test"};
end

to be:

handler myhandler
  a Button{text = "test"};
end

In general, I believe this is working as designed, since you cannot change the children of a container by assigning a new value to a variable that has already been added to the container. For example, neither of these work (you will only see 'here1' printed in the design view, and you can see 'here2' by clicking the button in preview view:

handler H7 type RUIhandler{initialUI =[ui
            ], onConstructionFunction = start, cssFile = "css/TestRUI.css", title = "H7"}

    ui Box{ children = [var2] };
    var2 Button{onClick ::= handleOnClick};

    function start()
    	SysLib.writestdout("here1");
    	var2 = new Button{text = "foo"};
    end
    
    function handleOnClick(event Event in)
    	SysLib.writestdout("here2");
    	var2 = new Button{text = "bar"};
    end
    	
end

I believe we will close this defect, but I will review it with the rest of the team before making the change.

Comment 13 fahua jin 2012-01-12 20:23:59 EST

Brian,

I searched the 'new' statement in our widget libraries of RBD 8013, and found few cases that using this approach to initialize the instance.

Project com.ibm.egl.rui_4.0.1
1) In file com.ibm.egl.rui.history.History.egl, 
34: frame = new Widget { tagName="iframe", visibility="hidden", position="absolute", width=0, height=0 };

2) In file DebugView.egl, 
29: layoutData = new GridLayoutData{ row = 1, column = 1 }, 

3) We allow to initialize the instance of external Java classes by this way, such as in file XMLLib.egl,
123: writer FileWriter = new FileWriter(file); 

Also, I can find lots of throw exception statements as following in both RBD & EDT, but they are not used in anonymous way.
throw new RuntimeException {message = messageValue, messageId = "CRRUI2004E"};

I personally is confused by the usage of the new statement, maybe we should have a document that clearly declares how to use the new statement if we want to design as current approaches.

(In reply to comment #12)
> I discussed this with Paul Harmon, and we will not update the IR to convert the
> following:
> 
> handler myhandler
>   a Button = new Button{text = "test"};
> end
> 
> to be:
> 
> handler myhandler
>   a Button{text = "test"};
> end
> 
> In general, I believe this is working as designed, since you cannot change the
> children of a container by assigning a new value to a variable that has already
> been added to the container. For example, neither of these work (you will only
> see 'here1' printed in the design view, and you can see 'here2' by clicking the
> button in preview view:
> 
> handler H7 type RUIhandler{initialUI =[ui
>             ], onConstructionFunction = start, cssFile = "css/TestRUI.css",
> title = "H7"}
> 
>     ui Box{ children = [var2] };
>     var2 Button{onClick ::= handleOnClick};
> 
>     function start()
>         SysLib.writestdout("here1");
>         var2 = new Button{text = "foo"};
>     end
> 
>     function handleOnClick(event Event in)
>         SysLib.writestdout("here2");
>         var2 = new Button{text = "bar"};
>     end
> 
> end
> 
> I believe we will close this defect, but I will review it with the rest of the
> team before making the change.

Comment 14 Huang Ji Yong 2012-01-12 21:43:07 EST

Hi Rocky,
The new expression is supported. The samples you listed also work in EDT. That is not the problem.

The problem is the forward reference in the initialization part. Take the sample EGL file as an example:
This snippet of code does not work, because Button3 was assign to a new Button after it appended to Box, so the children of the box is an empty Button
	Box4 Box{ padding=8,children = [ Button3 ] };
	Button3 Button = new Button{text = "New button with setting block"};
This snippet of code will work
        Button3 Button = new Button{text = "New button with setting block"};
        Box4 Box{ padding=8,children = [ Button3 ] };

The first snippet of code can be understood as the following code, in which Button3 is reassigned. Brian and I believe that this is not a bug.

Button3 Button{};
Box4 Box{ padding=8,children = [ Button3 ] };	
Button3 = new Button{text = "New button with setting block"};

(In reply to comment #13)
> Brian,
> 
> I searched the 'new' statement in our widget libraries of RBD 8013, and found
> few cases that using this approach to initialize the instance.
> 
> Project com.ibm.egl.rui_4.0.1
> 1) In file com.ibm.egl.rui.history.History.egl, 
> 34: frame = new Widget { tagName="iframe", visibility="hidden",
> position="absolute", width=0, height=0 };
> 
> 2) In file DebugView.egl, 
> 29: layoutData = new GridLayoutData{ row = 1, column = 1 }, 
> 
> 3) We allow to initialize the instance of external Java classes by this way,
> such as in file XMLLib.egl,
> 123: writer FileWriter = new FileWriter(file); 
> 
> Also, I can find lots of throw exception statements as following in both RBD &
> EDT, but they are not used in anonymous way.
> throw new RuntimeException {message = messageValue, messageId = "CRRUI2004E"};
> 
> I personally is confused by the usage of the new statement, maybe we should
> have a document that clearly declares how to use the new statement if we want
> to design as current approaches.
> 
> (In reply to comment #12)
> > I discussed this with Paul Harmon, and we will not update the IR to convert the
> > following:
> > 
> > handler myhandler
> >   a Button = new Button{text = "test"};
> > end
> > 
> > to be:
> > 
> > handler myhandler
> >   a Button{text = "test"};
> > end
> > 
> > In general, I believe this is working as designed, since you cannot change the
> > children of a container by assigning a new value to a variable that has already
> > been added to the container. For example, neither of these work (you will only
> > see 'here1' printed in the design view, and you can see 'here2' by clicking the
> > button in preview view:
> > 
> > handler H7 type RUIhandler{initialUI =[ui
> >             ], onConstructionFunction = start, cssFile = "css/TestRUI.css",
> > title = "H7"}
> > 
> >     ui Box{ children = [var2] };
> >     var2 Button{onClick ::= handleOnClick};
> > 
> >     function start()
> >         SysLib.writestdout("here1");
> >         var2 = new Button{text = "foo"};
> >     end
> > 
> >     function handleOnClick(event Event in)
> >         SysLib.writestdout("here2");
> >         var2 = new Button{text = "bar"};
> >     end
> > 
> > end
> > 
> > I believe we will close this defect, but I will review it with the rest of the
> > team before making the change.

Comment 15 fahua jin 2012-01-12 21:48:53 EST

Jiyong,

Could you please set a appropriate subject for the defect? Thanks.

(In reply to comment #14)
> Hi Rocky,
> The new expression is supported. The samples you listed also work in EDT. That
> is not the problem.
> 
> The problem is the forward reference in the initialization part. Take the
> sample EGL file as an example:
> This snippet of code does not work, because Button3 was assign to a new Button
> after it appended to Box, so the children of the box is an empty Button
>     Box4 Box{ padding=8,children = [ Button3 ] };
>     Button3 Button = new Button{text = "New button with setting block"};
> This snippet of code will work
>         Button3 Button = new Button{text = "New button with setting block"};
>         Box4 Box{ padding=8,children = [ Button3 ] };
> 
> The first snippet of code can be understood as the following code, in which
> Button3 is reassigned. Brian and I believe that this is not a bug.
> 
> Button3 Button{};
> Box4 Box{ padding=8,children = [ Button3 ] };    
> Button3 = new Button{text = "New button with setting block"};
> 
> (In reply to comment #13)
> > Brian,
> > 
> > I searched the 'new' statement in our widget libraries of RBD 8013, and found
> > few cases that using this approach to initialize the instance.
> > 
> > Project com.ibm.egl.rui_4.0.1
> > 1) In file com.ibm.egl.rui.history.History.egl, 
> > 34: frame = new Widget { tagName="iframe", visibility="hidden",
> > position="absolute", width=0, height=0 };
> > 
> > 2) In file DebugView.egl, 
> > 29: layoutData = new GridLayoutData{ row = 1, column = 1 }, 
> > 
> > 3) We allow to initialize the instance of external Java classes by this way,
> > such as in file XMLLib.egl,
> > 123: writer FileWriter = new FileWriter(file); 
> > 
> > Also, I can find lots of throw exception statements as following in both RBD &
> > EDT, but they are not used in anonymous way.
> > throw new RuntimeException {message = messageValue, messageId = "CRRUI2004E"};
> > 
> > I personally is confused by the usage of the new statement, maybe we should
> > have a document that clearly declares how to use the new statement if we want
> > to design as current approaches.
> > 
> > (In reply to comment #12)
> > > I discussed this with Paul Harmon, and we will not update the IR to convert the
> > > following:
> > > 
> > > handler myhandler
> > >   a Button = new Button{text = "test"};
> > > end
> > > 
> > > to be:
> > > 
> > > handler myhandler
> > >   a Button{text = "test"};
> > > end
> > > 
> > > In general, I believe this is working as designed, since you cannot change the
> > > children of a container by assigning a new value to a variable that has already
> > > been added to the container. For example, neither of these work (you will only
> > > see 'here1' printed in the design view, and you can see 'here2' by clicking the
> > > button in preview view:
> > > 
> > > handler H7 type RUIhandler{initialUI =[ui
> > >             ], onConstructionFunction = start, cssFile = "css/TestRUI.css",
> > > title = "H7"}
> > > 
> > >     ui Box{ children = [var2] };
> > >     var2 Button{onClick ::= handleOnClick};
> > > 
> > >     function start()
> > >         SysLib.writestdout("here1");
> > >         var2 = new Button{text = "foo"};
> > >     end
> > > 
> > >     function handleOnClick(event Event in)
> > >         SysLib.writestdout("here2");
> > >         var2 = new Button{text = "bar"};
> > >     end
> > > 
> > > end
> > > 
> > > I believe we will close this defect, but I will review it with the rest of the
> > > team before making the change.

Comment 16 Huang Ji Yong 2012-01-17 21:43:15 EST

Close this defect according to comment 12. Reopen it if you find any issues.

Comment 17 Brian Svihovec 2012-01-18 15:04:31 EST

I agree with closing this defect as Won't Fix.  

NOTE: This is the same behavior as RBD, with a twist related to the fact that in RBD you have to add "{}" after the widget to instantiate a new instance.  To remove this twist from the discussion, I have the following example, which displays the same behavior in RBD and EDT:

                varA Button{text = "A"};
		varB Button[] = [varA];
		varA = new Button{text = "B"};

		SysLib.writestdout("test: " + (varB[1] as Button).text);

At runtime, this prints "test: A".

The assignment of varA to the varB array places the original button object in the array.  VarA is then assigned to a new button, but the contents of varB remains unchanged.

Comment 18 fahua jin 2012-01-19 00:24:46 EST

Close the defect based on the comments from Brian.

Comment 19 Huo Zhen Zhong 2012-01-30 22:03:07 EST

*** Bug 364256 has been marked as a duplicate of this bug. ***