Bug 275032 - [itds] unhelpful message for itd of default ctor onto a type
Summary: [itds] unhelpful message for itd of default ctor onto a type
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: DEVELOPMENT   Edit
Hardware: PC Windows NT
: P3 normal (vote)
Target Milestone: 1.6.5   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-05 13:50 EDT by Andrew Clement CLA
Modified: 2009-05-20 14:06 EDT (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Clement CLA 2009-05-05 13:50:42 EDT
The default constructor is created automatically if not defined.  An ITD of the default constructor can clash with it and gives an unhelpful message.  Effectively the ITD is providing the ctor so we shouldn't be creating a default one.
Comment 1 Andrew Clement CLA 2009-05-05 14:47:32 EDT
this is tricky.  Whether the constructor was created by the compiler or not is hard to determine, there are no tags in the bytecode.  When the Eclipse compiler builds it the flag isDefaultConstructor bitflag is set but that only lasts through until the type has gone through the compiler pipeline and been converted to bytecode.  So testing that flag works as long as the aspect goes through the pipeline before the target type.  However, for some orderings it won't work and in the case of separate compilation (when the constructor is first encountered as a BcelMethod), it cannot be used to determine whether it is OK to overwrite the constructor with the ITD.
Comment 2 Andrew Clement CLA 2009-05-05 18:22:26 EDT
ok - creative solution but it works.

Two scenarios where we need to recognize the default constructor:
- for an eclipse resolved member we can just check the bit flag that JDT sets when creating it
- for a bcel resolved member based on the bytecode we can observe whether it has a MethodDeclarationLineNumber attribute.  If it does then it was written in the source, if it doesn't then it was generated.

This passes all the tests - but I can still think of one failure scenario - however it is rare.  If some code is compiled with javac and that code contains a no-arg constructor, and *then* an aspect is written to apply to it that also defined a no-arg constructor - then the itd will overwrite the constructor in the target type.

need to slightly extend the fix I think - I don't mind if that final javac case is an error but it shouldn't silently overwrite.
Comment 3 Andrew Clement CLA 2009-05-05 19:41:56 EDT
decided that the unusual failure case can be ignored for now.  If showWeaveInfo or ajdt is being used, it will be apparent what happened.
Comment 4 Andrew Clement CLA 2009-05-05 22:38:40 EDT
done some testing of this inside AJDT.  It works... but only for the full build. An incremental build doesn't detect when an error situation is triggered or fixed.  For example, if the ITD of the no-arg ctor is onto a type and then the user adds a no-arg ctor to the type and saves it, no error is reported.
Comment 5 Andrew Clement CLA 2009-05-06 13:42:17 EDT
standalone regression test for incremental works as expected, argh!

debugging it in AJDT there are two problems here:
- the error is reported but doesn't make it back to the UI
- the default ctor is removed regardless of whether there was an error !
Comment 6 Andrew Clement CLA 2009-05-06 14:56:39 EDT
ok - latter problem is fixed, the no-arg ctor is not accidentally deleted if the ITD does clash.

The first problem is because when the target class is modified to break the ITD, the error message is recorded against the aspect.  AJDT only shows messages recorded against types it knows have changed, so the error does not appear.  The error can easily be recorded against the target ctor that just broke the ITD (with a secondary location allowing navigation to the ITD).

However, there is a new/strange problem now.  On a full build (in the broken state), there is no relationship in the map.  But on a breaking incremental build the relationship remains - I don't quite understand why it wasnt removed during incremental preparation

what a can o worms
Comment 7 Andrew Clement CLA 2009-05-20 12:36:43 EDT
relationship problem was fixed under another bug.  But now we have some real wierdness in the ResolvedType.compareToExistingMembers() method.  So I'm rewriting that piece of code - it is poorly specified and there is no clue from the name what the boolean return value means.  Does it mean it matched an existing member or that it didn't match an existing member so is OK to continue treating as a valid itd?

Renamed to clashesWithExistingMember() - returns true if there is a clash.  This may not be an error (since the ITD is considered a 'default implementation').
Comment 8 Andrew Clement CLA 2009-05-20 12:57:38 EDT
much better.  modified error reporting to add errors at both ends of the problem, now always obvious there is a problem - and when there is one then any user written ctor in the target is not overwritten by the aspect (should the user choose to run it in a broken state)
Comment 9 Andrew Clement CLA 2009-05-20 14:06:23 EDT
alright, done enough here for now. committed everything.  Slightly concerned about message duplicates but they will only come out when the code is broken anyway.