Discussion: Concept of printing imports and qualified names

[pvojtechovsky]

I am trying to fix tests in #2621, which marked a CtTypeAccess of enum field as implicit, when such field is statically imported.

Now I would need a concept/specification of how the spoon printer should behave in
A) autoImport = false -> MinimalImportScanner
B) autoImport = true -> ImportScannerImpl
C) snipermode

Example of source code:

import static spoon.reflect.path.CtRole.NAME;
import spoon.reflect.path.CtRole;
...
CtRole r1 = NAME;              //CtTypeAccess.isImplicit == true
CtRole r2 = CtRole.NAME;  //CtTypeAccess.isImplicit == false
CtRole r3 = spoon.reflect.path.CtRole.NAME;

In my understanding the autoImport = false means that all what can use fully qualified name will use it. So it should print:

// no import here - if possible
...
CtRole r1 = spoon.reflect.path.CtRole.NAME;
CtRole r2 = spoon.reflect.path.CtRole.NAME;
CtRole r3 = spoon.reflect.path.CtRole.NAME;

It means isImplicit is ignored and always understood as false.

Then the autoImport = true means that
B1) we try to print the sources in the way how they are modeled. It means we print or not print qualified name depending on implicit attribute
B2) we try to automatically import as much as possible. We import (including static import?) as much as possible

And the sniper mode means we try to print the sources in the way how they are modeled. It means we print or not print qualified name depending on implicit attribute. It expects that model (implicit=true/false) reflects origin sources.

[monperrus]

Initially, autoimports meant B1.

Now that we have the sniper mode, which is basically B1, we can more autoimport to B2 and make autoimport independent of implicit values. This will likely simplify the autoimport code quite a lot.

[pvojtechovsky]

Now that we have the sniper mode, which is basically B1, we can more autoimport to B2

It will not work, because Sniper mode uses internally standard printing (auto import) and just wraps it to minimize changes. So the standard printing should try to produce the origin code as much as possible, then also Sniper mode will print source more matching to origin code.

So I suggest to Use B1 in standard auto import mode too.

This will likely simplify the autoimport code quite a lot.

The complexity and correctness of printer and autoimport (ImportScannerImpl) code is a relevant topic here too. Both implementations DJPP and ImportScannerImpl must decide whether a type will be imported or not in the same way. So they should somehow use same algorithm. That leads to an idea of next refactoring, which will keep that "to import or not to import" algorithm in DJPP only the ImportScannerImpl will just follow the decisions made by DJPP.

WDYT?

[pvojtechovsky]

I am trying to understand what is the expected behavior of the printer and import scanner... to fix some problems.

If autoImport = false we print all references in fully qualified form. Excluding references to locally defined types, methods and fields.

If autoimport = true (or in sniper mode) we follow these rules:

/**
 * <ul>
 * <li>if the reference.isImplicit() == false the reference has to be printed inline,
 * 		so this occurrence doesn't cause import. BUT only if it is possible (there is no conflict, etc.)
 * <li>if the reference.isImplicit() == true the reference should not be printed,
 * 		so it should be imported. BUT only if it is possible (there is no conflict, etc.)
 * </ul>
 *
 * When printing references, it is not relevant whether import existed in origin file or not. Only {@link CtElement#isImplicit()} drives printing.
 * The list of origin imports should influence only the order of printed imports.
 *
 * The spoon model builder should set reference.setImplicit(X) following the origin source code, so we know later,
 * whether reference exists in source code or whether it doesn't exist and therfore it must be imported or is locally available.
 */

@surli I guess you know the ImportScannerImpl the best of us. Could you please read this comment and give me feedback whether it is correct (for future) or not?

First: I am not asking about current (sometime probably buggy) implementation, I am asking about future concept, which should help me to fix it in correct way.

Second: How much is the current implementation fitting to the concept above? Does it needs deep refactoring or just little fix?

[surli]

Hi @pvojtechovsky I don't think I completely agree with the final rule:

• The spoon model builder should set reference.setImplicit(X) following the origin source code, so we know later, whether reference exists in source code or whether it doesn't exist and therfore it must be imported or is locally available.

I'm ok that on first traversal the model builder shoud set the setImplicit but then the ImportScanner could also change it, if a conflict has been found for example.

Second: How much is the current implementation fitting to the concept above? Does it needs deep refactoring or just little fix?

I think the work on the pretty printer should be quite easy, as everything is now based on the impliciteness that has been computed before.

Now regarding the ImportScanner itself, I'm not sure it will be so easy:

1. you need to refactor it to remove the check against existing imports,
2. the MinimalImportScanner and ImportScanner are currently tight, I'm not sure it will be the case in the future as the rules are not the same anymore
3. concerning the conflict management, I'm not sure either it will be the same: as I was using the existing list of imports to compute conflicts, I could find a conflict even if the conflicting reference was after the current element, but now as it will be based only on the impliciteness, you might need to traverse the model twice to detect conflict, or to obey on rules such as "the first element on the model has the priority", which might change existing codes, that we would want to avoid.

[pvojtechovsky]

Hi Simon,

but then the ImportScanner could also change it, if a conflict has been found for example.

I would prefer to remember information about conflict in some printing context. The modification of the original model is not good in these cases

• when the refactoring is in progress - may be the next refactoring step will remove conflict ...?
• when different import scanners are used

The purpose of ImportScanner is not to validate model, so it should not change the model.

WDYT?

refactoring of ImportScanner ... I'm not sure it will be so easy

@surli, many thanks for your notes regarding refactoring. It is very helpful to see the bigger challenges which are waiting there.

[surli]

The purpose of ImportScanner is not to validate model, so it should not change the model.

I think you rise an important point that we need to clarify: the exact purpose of the ImportScanner.
There are two ways of seeing it:

1. a tool for the pretty printer, to nicely print the references and which uses imports
2. or a model transformation tool, which will compute the import of the model based on the references

For me, at the beginning the purpose of the ImportScanner was only 1, but I have the feeling that the usage of our user led to have 2: that's why I pushed to have CtImport inside the model and so one.

So, I'd say it should allow both:

• it remembers information about conflict and needed model transformation to fix them
• at the end (before printing?), it applies those transformation on the model

[pvojtechovsky]

CtImport inside the model

I agree that since CtImport is in model situation changed. The simpler and probably the most powerful approach looks like this:

I1) pretty printer doesn't need import scanner at all. It can simply print sources following current model.

Note: Everything is defined in model. There are imports, which has to be printed and there is attribute isImplicit, which defines whether scope name (package, type, ...) has to be printed or not. Client can influence everything simply by change in the model. The only "problem / feature" is that it is possible to print inconsistent sources. From #2397 this behavior is "feature" and I personally like that approach too.

I2) import scanner will modify model (adds/removes imports and changes isImplicit) by way the sources are compilable if fixed model is printed later.

note: import scanner (may be we should call it import computer/validator/...) can be used by clients as optional step before sources are printed.

WDYT? Is it the way we want to go?

[surli]

note: import scanner (may be we should call it import computer/validator/...) can be used by clients as optional step before sources are printed.

Actually I like the idea of having in Spoon a list of "processors" that would validate a processed model before its printing. So we could imagine that the ImportScanner would be a first "default" validator in that list, but users are able to change the list to add their own. On a long term view it would allow us to develop new ways to check the model before printing.

Sorry, it wasn't exactly your point, but it makes me think about that.

WDYT? Is it the way we want to go?

So for me, yeah absolutely.

[pvojtechovsky]

Super! So we have a new powerful concept for PrettyPrinter and ImportScanner for the case of autoImport=true.
But how that should work with autoImport=false?

I guess the printing of model in mode autoImport=false should not modify model, because such printing is done with each CtElement#toString() call ... and it would be really bad if CtElement#toString() would change the model ... because during debugging the client's would wonder why model is changing after they looked at element using some debugger object viewer.

So we should be able to configure PrettyPrinter by way it ignores isImplicit and ignores imports and simply prints fully qualified names whenever is it possible.

... but that is completely different approach - different concept -> more code needed, more maintenance ...

Do you have any better idea?

Or do you think that some clients might need to modify model by way all isImplicit is set to false and imports are removed from the model?

[surli]

For me both mode are working the same way:

The model is processed, transformed, and validated before being printed.
During the validation phase an ImportScanner is by default triggered, a DefaultImportScanner if autoimports=true, a MinimalImportScanner else.
Those scanner are able to change the model before printing.

So we stand on those principles:

• the prettyprinter always rely on the model, without changing it
• the validation phase might change the model in order to fix it (but we can also imagine other ImportScanner which only warn the users about problems in their model, like name conflicts)

WDYT?

[pvojtechovsky]

I like it.

But that needs a change in spoon behavior concerning CtElement#toString(). May be we should simply print the model as it is, without any import scanners.
WDYT?

[surli]

But that needs a change in spoon behavior concerning CtElement#toString(). May be we should simply print the model as it is, without any import scanners.

I personally think it's better: as you mentioned I had many times the problem when debugging a scanner, of an element changing because of the ImportScanner in CtElement#toString(). So I think it should be removed.

[pvojtechovsky]

This issue is the most urgent for me now, so I will probably make time to have a look at it next days.

@monperrus could you give us feedback whether new concepts discussed here are OK from your point of view?

[monperrus]

everything OK, we make good progress. I propose to change the title of the issue because we discuss more than isImplicit.

[pvojtechovsky]

Do you agree that CtElement#toString() changes behavior and will print the model in the form it is ... so if origin sources use just type name without package, then toString will print it same?
I am asking because it probably means many changes in tests like:
AnnotationTest#testFieldAndMethodInAnnotation

		assertEquals("java.lang.String value = \"\";", fieldValue.toString());

has to be replaced by

		assertEquals("String value = \"\";", fieldValue.toString());

because origin source code is

String value = "";

Will you accept such changes in tests?

[surli]

It makes sense for me: the test is even closer to the reality so I find it better. If we really want to check the fully qualified type, then we can always get its reference and check it.

[pvojtechovsky]

JDTBasedSpoonCompiler#buildModel actually collects imports from sources only when getFactory().getEnvironment().isAutoImports()==true.
I suggest to collect imports always. And to remove them later by "no import" model validator, which prepares model for printing with fully qualified names without imports.
Do you agree? to collect imports always at this place?

[surli]

I suggest to collect imports always. And to remove them later by "no import" model validator, which prepares model for printing with fully qualified names without imports.
Do you agree? to collect imports always at this place?

That would mean if an user use Spoon with "no import", and get the imports from the model before printing it, he'll get a list of imports.
It's not necessarily a bug, but it might raise the question of "when the validators should be triggered".

So ok for now to always collect and to remove them in the no import validator. We'll see later if we need to trigger the validators at different places in the process or not, it might be the subject of another discussion.

[pvojtechovsky]

Do you agree that CtElement#toString() changes behavior and will print the model in the form it is ... so if origin sources use just type name without package, then toString will print it same?

I changed my mind. I found algorithms, where it is necessary to have CtElement printed to String with fully qualified names and it would be bad at this place to apply model validator which removes implicit attributes from model - to print it fully qualified.

So I suggest to make it more backward compatible. It means CtElement#toString() will print in fully qualified mode (same like before). I added DJPP#setForceFullyQualified(boolean), which tells DJPP that it should ignore isImplicit and print full qualified without changing model. So now it is possible.

But then there are places where client needs to see how CtElement will really look like (follow is implicit). So I added CtElement method

/**
 * @param forceFullyQualified if true then fully qualified names are used even for implicit elements.
 * 	if false then print follows implicitness of elements
 *
 * @return pretty printed source code of this element
 */
String print(boolean forceFullyQualified);

WDYT?

[surli]

Not sure to understand:

It means CtElement#toString() will print in fully qualified mode (same like before)
[...]
I added CtElement method String print(boolean forceFullyQualified);

If the CtElement#toString always print in FQN, why do you need another method with a boolean to force the FQN printing?

[surli]

I just checked the PR so I understand better what you're doing. But still, I'm not sure to agree: IMO the long-term goal is to simplify how we manage those cases, so possibly to get rid of those corner-cases where we need to pretty-print in FQN etc.

So following this vision, I'd prefer to keep a CtElement#toString calling the pretty-printer normally (without forcing the FQN). And to add a method for calling the print with forcing pretty-printer. It forces you to make change in the code at every place the FQN is really needed for CtElement, but it also shows us those are corner cases. WDYT?

[pvojtechovsky]

Thanks for your constructive feedback! I agree that having

1. CtElement#toString() which prints in FQN mode
2. CtElement#print(true) which is same like toString
3. CtElement#print(false) which prints sources as they are defined

is too complicated. So I like idea to have only two methods. One for FQN and second for normal printing.

I actually tend to opposite solution:

1. CtElement#toString() prints in FQN mode (ignores isImplicit==true), means will always show what is in element, even if it is implicit element.
2. CtElement#print() which prints sources as they are defined (follows isImplicit), means will return empty string for implicit elements.

This solution

• is more backward compatible
• is better for debugging where toString is by default used, where FQN representation tells developer more then empty string.
• the method CtElement#print() will do the same like DJPP printing ... so even names and behavior is nearer.

WDYT?

[monperrus]

Closing, the discussion continues on #2683 and #2638