Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#2318] Clean AllAssociation when removing field with active annotation #2776

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dvojtise
Copy link
Contributor

@dvojtise dvojtise commented Aug 9, 2023

This PR contributes to #2318

  • adds a test highlighting the issue (the test testresourceStorageWrite fails if the fix below isn't applied)
  • applies the suggested correction in org/eclipse/xtend/core/macro/declaration/JvmBasedDeclarations.xtend

@dvojtise dvojtise changed the title [#2318] Clean AllAssociation when removing field [#2318] Clean AllAssociation when removing field with active annotation Aug 9, 2023
@@ -1320,6 +1320,8 @@ class JvmFieldDeclarationImpl extends JvmMemberDeclarationImpl<JvmField> impleme
}

override remove() {
compilationUnit.jvmModelAssociator.removeAllAssociation(delegate.type)
Copy link
Contributor

@szarnekow szarnekow Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be other removevAllAssociations(..) missing in this file, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes probably on other kind of remove
but since it make silently crash other remove (see the new failed test. after the first remove. the other ones aren't processed)

I'm not sure anymore that it should be done that way

I must admit I don't master xtend internal structures and I'm not efficient in finding a solution for cleaning these structures on all remove situations

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give it a stab later this week.

@cdietrich
Copy link
Member

can you also have a look at the failing test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants