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

review: fix: Unsettable properties sets no value or sets different primary value #1893

Merged
merged 3 commits into from
Mar 6, 2018

Conversation

pvojtechovsky
Copy link
Collaborator

Here is the test, which reports inconsistencies in Spoon model role get/set.

Generally there are these issues:
P1) Unsettable property CtXxxReference#comment<java.util.List<spoon.reflect.code.CtComment>> sets the value
P2) UnsettableProperty CtEnumValue|CtLocalVariable|CtField#assignment<spoon.reflect.code.CtExpression> sets the value into different role defaultExpression
P3) UnsettableProperty CtLambda#thrown<java.util.Set<spoon.reflect.reference.CtTypeReference<? extends java.lang.Throwable>>> sets the value
P4) Settable field CtCatchVariable#type<spoon.reflect.reference.CtTypeReference> is accessible by multiType too

I am not sure whether we should solve these problems. May be they are features for somebody?

Solutions might be

P1

to implement empty CtReference#setComment().

P2

??

P3

to implement empty CtLambda#setThrownTypes

P4

to mark role CtCatchVariable#TYPE as derived

WDYT?

@pvojtechovsky pvojtechovsky force-pushed the testRoleInconsistencies branch from ff1d307 to a579f36 Compare March 3, 2018 08:35
@monperrus
Copy link
Collaborator

OK for me. For P2, I would simply remove the @DerivedProperty (the API documentation is clear about the goal).

@pvojtechovsky
Copy link
Collaborator Author

The @DerivedProperty on getter is no problem. The problem is meaning of @UnsettableProperty. I understand the meaning: "you can call this method, but it will do nothing" ... but this contract is broken by code like

class CtFieldImpl { ...
	@UnsettableProperty
	public <C extends CtRHSReceiver<T>> C setAssignment(CtExpression<T> assignment) {
		setDefaultExpression(assignment);
		return (C) this;
	}

May be we can live with that, I just wanted to be sure that we speak about the same thing.

@monperrus
Copy link
Collaborator

My bad, I indeed meant UnsettableProperty:

For P2, I would simply remove the @UnsettableProperty (the API documentation is clear about the goal).

@pvojtechovsky pvojtechovsky changed the title HELP: test: Assure role get/set consistency WIP: test: Assure role get/set consistency Mar 3, 2018
@pvojtechovsky
Copy link
Collaborator Author

I am mainly not sure with CtLambdaImpl#setThrownTypes, where I deleted code added by You ;-)
A) it is correct, it has to be deleted
B) we have to remove UnsettableProperty from CtLambda#setThrownTypes

So if you agree with A), then this is finished

@pvojtechovsky pvojtechovsky changed the title WIP: test: Assure role get/set consistency review: fix: Unsettable properties sets no value or sets different primary value Mar 3, 2018
@monperrus
Copy link
Collaborator

OK for me. Will merge it.

@monperrus monperrus merged commit 58978c6 into INRIA:master Mar 6, 2018
@pvojtechovsky pvojtechovsky deleted the testRoleInconsistencies branch March 10, 2018 13:09
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.

2 participants