review: feature: SourcePosition#isValidPosition(), throw exception when accessing invalid position

[pvojtechovsky]

This is alternative to #1963

1. introduces new Concept Source

/**
 * This interface represents the origin source of a program element
 */
public interface Source extends Serializable {

	/**
	 * Gets the file for this position.
	 */
	File getFile();

	/**
	 * Gets the compilation unit for this position.
	 */
	CompilationUnit getCompilationUnit();
}

2. SourcePosition extends Source
3. CtElement#getPosition() returns null if source position is not known. It is DerivedProperty now.
4. there is new CtElement#getSource() and setSource(Source)
5. there is new CtElement#getCompilationUnit() which return not null even for cases when NoSourcePosition was used before.
6. all CtElements which belongs to one Compilation unit share one SourceImpl (before each had own instance of PartialSourcePositionImpl) - less memory needed
7. removes NoSourcePosition and PartialSourcePosition.

It is not backward compatible for the code which expected NoSourcePosition or asked getSourceStart() >= 0, ... , but:

• most of the modified code was incorrect anyway, because such check did not counted with new PartialSourcePosition , ...
• I think it is now more KISS and client's friendly for future code.

WDYT?

[monperrus]

3. CtElement#getPosition() returns null if source position is not known. It is DerivedProperty now.

Returning null is a bad and dangerous practice. I would prefer not to go in this direction.

6. all CtElements which belongs to one Compilation unit share one SourceImpl (before each had own instance of PartialSourcePositionImpl) - less memory needed

That's good!

4. there is new CtElement#getSource() and setSource(Source)

What would be the difference with getPosition?

[monperrus]

Sidenote: when it is no backward-compatible, please avoid prefixing the PR with refactoring. If it is a refactoring, by convention/definition, it does not change the behavior and thus there are no assertion modification or deletion in src/test.

[pvojtechovsky]

Returning null is a bad and dangerous practice. I would prefer not to go in this direction.

What do you suggest? Note that existing approach with NoSourcePosition and PartialSourcePosition has some problem - see problems discussed in #1963

[pvojtechovsky]

there is new CtElement#getSource() and setSource(Source)

What would be the difference with getPosition?

I do not understand, the question. But let's discuss first the alternative for getPosition returns null. It is the main point of this PR, so any change here will completely change whole solution ;-)

[monperrus]

Note that existing approach with NoSourcePosition and PartialSourcePosition has some problem - see problems discussed in #1963 <#1963>

A lot of things were discussed in #1963. What is main problem you're referring to?

[pvojtechovsky]

main problem?

nobody really knows how to correctly ask whether value returned by CtElement#getPosition contains source position or not.

With this PR it is clear
if (element.getPosition() != null) { /*then you can use position*/} else {/*it is not available*/}

But what is correct without this PR???

[monperrus]

What you proposed sounds good to me

boolean isValid() {
  return this instanceof NoSourcePosition == false;
}

One method, clear responsibility, encapsulates implementation detail, no null value

[pvojtechovsky]

I have two questions, before I can change this PR:

1. Do you have better name for that isValid method? May be

• isPositionValid
• isStartEndValid
• ...
  because for example compilation unit and source File are valid always ...

2. what should return getStart(), ... when isValid() == false ??
   A) -1 like till now
   B) throw exception - like @tdurieux suggests?

I prefer throw exception too. It avoids ugly bugs...

[tdurieux]

1. what about isValidPosition?

2. A prefer the option B

[pvojtechovsky]

Thanks Thomas for your opinion, I agree with you.
@monperrus please give us feedback too because throwing exceptions is not backward compatible ... so will you accept that solution?

[monperrus]

agree with @tdurieux.

[tdurieux]

the path to Source is not found during the javadoc generation

[monperrus]

Looking forward to it, it should fix the regression identified in Casper: https://travis-ci.org/Spirals-Team/casper/builds/367789415

[pvojtechovsky]

So we are three in this cinema ... looking at yellow circle ... and waiting for happy end :-))

[spoon-bot]

API changes: 1 (Detected by Revapi)

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-20180417.183645-43 / New API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-SNAPSHOT

	Method was added to an interface.
Old	none
New	method SourcePosition#isValidPosition()
Breaking	binary: non_breaking,

[monperrus]

thanks for the comprehensive refactoring

[monperrus]

I confirm that the regression has been fixed