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

Fully rewritten semantic annotations #835

Open
wants to merge 19 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@mayakwd
Collaborator

mayakwd commented May 14, 2018

Added lots of checks and fixes, rewritten the old one, added proper models preserving, improved references evaluation.

@mayakwd mayakwd requested a review from EricBishton May 14, 2018

@mayakwd

This comment has been minimized.

Collaborator

mayakwd commented May 14, 2018

Some leaks in the tests, also I've used new version of method from StringUtils, going to fix it soon. Anyway PR could be reviewed

@EricBishton

This comment has been minimized.

Member

EricBishton commented May 14, 2018

You realize that this is going to take a week to review properly, right? I'll probably review it in stages.

@EricBishton

This comment has been minimized.

Member

EricBishton commented Jun 8, 2018

@mayakwd Hi Ilya, I'm starting to review this now. Do you want to update it before I go too far?

@EricBishton

As a note to myself: I'm up to HaxeSemanticAnnotator.visitNewExpression().

haxe.semantic.method.body.not.allowed=Method body not allowed for methods declared in {0}
haxe.semantic.return.type.required=Return type required for method declared in {0}
haxe.semantic.parameter.type.required=Parameter type required for method declared in {0}
haxe.semantic.type.name.uppercase=Type name ''{0}'' must start with uppercase

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

Type name ''{0}" must start with an upper-case character.

haxe.semantic.cant.extend.self=Cannot extend self
haxe.semantic.variable.redefinition.not.allowed=Redefinition of variable ''{0}'' in subclass is not allowed. Previously declared at ''{1}''
haxe.semantic.package.not.correspond.file.path=Package name ''{0}'' does not correspond to the file path ''{1}''
haxe.semantic.package.name.uppercase=Package name ''{0}'' must start with a lower case character

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

lower-case

<tr><td>Expected:</td><td><b>{1}</b></td></tr>\
<tr><td>Found:</td><td>{0}</td></tr>\
</table></body></html>
haxe.semantic.more.arguments.than.expected=Found more arguments than expected

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

In general, all complete sentences should end in a period ('.').

haxe.semantic.not.allowed.override=Can't override static, inline or final methods
haxe.semantic.overrides.nothing=Method overrides nothing
haxe.semantic.duplicate.parameter.name=Duplicate parameter name ''{0}''
haxe.semantic.invalid.accessor.type=Invalid {0}ter accessor type. Only ''{0}'', ''default'', ''null'', ''never'', ''dynamic'' types allowed

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

... ''never'', and ''dynamic'' types allowed.

haxe.semantic.invalid.accessor.type=Invalid {0}ter accessor type. Only ''{0}'', ''default'', ''null'', ''never'', ''dynamic'' types allowed
haxe.semantic.accessor.method.not.found=Accessor method ''{0}'' not found
haxe.semantic.not.real.var=This field cannot be initialized because it is not a real variable
haxe.semantic.optional.mark.redundant=Optional mark is redundant when initial value specified

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

...initial value is specified.

haxe.semantic.field.has.different.accessors=Field has different property access than defined at ''{0}'' (''{1}'' should be ''<b>{2}</b>'')
haxe.semantic.field.type.required=Field type required by interface ''{0}'', should be ''{1}''
haxe.semantic.unexpected.parameter=Unexpected parameter
haxe.semantic.not.matching.arity=Not matching arity, expected {0} parameters count, got {1}

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

Arity mismatch: Expected {0} parameters, but found {1}.

haxe.semantic.final.static.var.init=Static final variable ''{0}'' must be initialized
haxe.semantic.property.cant.be.final=Property can not be final

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

...cannot...

# HAXE quickfixes
# {0} - Found type, {1} - Expected type
haxe.quickfix.add.cast=Add cast from ''{0}'' to ''{1}''
haxe.quickfix.remove.excessive.arguments=Remove excessive arguments

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

Remove excess arguments.

haxe.quickfix.setup.module.source.paths=Configure module source-paths
haxe.quickfix.rename.to=Rename to ''{0}''
haxe.quickfix.create.accessor.method=Create {1}ter-method ''{0}_{1}''
haxe.quickfix.add.optional.mark=Remove optional mark

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

Add optional mark.?

@@ -187,7 +252,7 @@ haxelib.dependency.list.separator=,
# HAXE Module settings dialog.
module.settings.synchronize.dependencies=Use Haxe project files and haxelib to update module dependencies and settings. (Project file changes update IDEA settings; IDEA does not update Haxe project files.)
module.settings.synchronize.dependencies.title=Automatically s&ynchronize dependencies and settings
module.settings.synchronize.dependencies.title=Automatically synchronize dependencies and settings

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

The ampersand is there to set the shortcut for the checkbox to Alt+Y.

@EricBishton

Note to self: Up to HaxeCreateConstructorFromCallFix.java.

if (ApplicationManager.getApplication().isUnitTestMode()) {
PsiElement body = model.getBodyPsi();
if (body != null) {
HaxeTypeResolver.getPsiElementType(model.getBodyPsi(), holder);

This comment has been minimized.

@EricBishton

EricBishton Jun 20, 2018

Member

What is the point of this block?

public void visitAnonymousType(@NotNull HaxeAnonymousType o) {
final HaxeAnonymousTypeBody body = o.getAnonymousTypeBody();
if (body == null) return;
HaxeTypeExtendsList extendsList = body.getTypeExtendsList();

This comment has been minimized.

@EricBishton

EricBishton Jun 20, 2018

Member

This is probably going to have to be revisited in the face of the new "type & type" syntax.

createErrorAnnotation(extendType, HaxeBundle.message("haxe.semantic.cant.extend.self"))
.registerFix(new HaxeDropExtendDeclaration(o, extendType));
} else if (!isAnonymousType(resolvedItem)) {
createErrorAnnotation(extendType, HaxeBundle.message("haxe.semantic.not.anonymous.type"))

This comment has been minimized.

@EricBishton

EricBishton Jun 20, 2018

Member

This seems wrong. An anonymous type can inherit from a non-anonymous type. In fact, it has to if it's going to inherit at all. Otherwise, it would be an embedded anonymous type, not an inheritance.

if (!parameterType.canAssign(argumentType)) {
int index = getIndexOfSuitableParameterAhead(parameters, parameterIndex, argumentType);
if (index != -1) {

This comment has been minimized.

@EricBishton

EricBishton Jun 20, 2018

Member

In this case, you are skipping ahead because a parameter is missing. Don't you want to add an error annotation (extra argument detected?) if we do this? And, perhaps only skip forward if the number of parameters is longer than the number of arguments presented? Otherwise, if two arguments are swapped, then we will report only "not enough parameters."

Actually, I don't think we'll even get that, because we don't check the argument count against the position at the end of this loop. We check it only against the total expected.

if (requiredParametersCount > argumentsCount) {
String message = HaxeBundle.message("haxe.semantic.0.arguments.expected.got.1", requiredParametersCount, argumentsCount);
createErrorAnnotation(context, message);

This comment has been minimized.

@EricBishton

EricBishton Jun 20, 2018

Member

We also need to put up an error if too many arguments are present.

HaxeTypeResolver.getPsiElementType(method.getBodyPsi(), holder);
private static boolean isSingleQuotesRequired(HaxeStringLiteralExpression o) {
if (o.getParent() instanceof HaxeObjectLiteralElement) return false;
return (o.getLongTemplateEntryList().size() > 0 || o.getShortTemplateEntryList().size() > 0) &&

This comment has been minimized.

@EricBishton

EricBishton Jun 20, 2018

Member

This also has to be sure that the string isn't the name of a key in a structure or map. Those are allowed to have $ in the identifier string and are never supposed to be interpolated, either in single or double quoted situations.

private static boolean isSingleQuotesRequired(HaxeStringLiteralExpression o) {
if (o.getParent() instanceof HaxeObjectLiteralElement) return false;
return (o.getLongTemplateEntryList().size() > 0 || o.getShortTemplateEntryList().size() > 0) &&
o.getFirstChild().textContains('"');

This comment has been minimized.

@EricBishton

EricBishton Jun 20, 2018

Member

Use o.getFirstChild().getText().startsWith('"');. Otherwise, escaped quotes will be detected.

if (element instanceof HaxeIdentifier || element instanceof HaxePsiToken || element instanceof HaxeStringLiteralExpression) return;
element.acceptChildren(this);
private String wrapWithHtmlError(String string) {
String color = UIUtil.isUnderDarcula() ? "FF6B68" : "red";

This comment has been minimized.

@EricBishton

EricBishton Jun 20, 2018

Member

This color should be in the settings dialog. It's OK to create a "technical debt" issue for this one, because it's going to take UI work that I'd rather see done in a separate PR.

}
}
}

This comment has been minimized.

@EricBishton

EricBishton Jun 20, 2018

Member

Do you want an isAvailable() that checks whether a class body can be found?

List<HaxeNamedComponent> elements =
HaxeElementGenerator.createNamedSubComponentsFromText(project, builder.toString());
PsiElement anchor = classBody.getLastChild();

This comment has been minimized.

@EricBishton

EricBishton Jun 20, 2018

Member

Throw an // TODO: Use (or define) style setting to determine order. comment in here, so it will be easier to find. For now, putting them in the last place is OK.

@EricBishton

Okay, Ilya, I am done with this review in the current state. I'm excited about seeing this functionality.

@Nullable Editor editor,
@NotNull PsiElement startElement,
@NotNull PsiElement endElement) {
// TODO ilyam

This comment has been minimized.

@EricBishton

EricBishton Jun 21, 2018

Member

Maybe not register this fix in the visitor until we have an implementation?

if (!missingFields.isEmpty() || !missingMethods.isEmpty()) {
String tooltip = buildMissingMembersMessage(implementedInterface, missingMethods, missingFields);
Annotation annotation = createErrorAnnotation(implementedInterface.getPsi().getTextRange(), tooltip, true);
annotation.registerFix(new HaxeImplementMembersFix(haxeClass));

This comment has been minimized.

@EricBishton

EricBishton Jun 21, 2018

Member

HaxeImplementMembersFix isn't complete yet, so comment out this fix or implement that?

@NotNull PsiFile file,
@NotNull PsiElement startElement,
@NotNull PsiElement endElement) {
return rootBasedPackageName != null;

This comment has been minimized.

@EricBishton

EricBishton Jun 21, 2018

Member

&& null != PsiDocumentManager.getInstance(project).getDocument(packageStatement.getContainingFile())

@NotNull PsiFile file,
@NotNull PsiElement startElement,
@NotNull PsiElement endElement) {
return startElement instanceof HaxeParameter;

This comment has been minimized.

@EricBishton

EricBishton Jun 21, 2018

Member

&& ((HaxeParameter)startElement).isOptional() or similar.

@@ -86,6 +90,16 @@ public PsiElement getDeclarationScope() {
return null;
}
@Override
public HaxeParameterModel getModel() {
HaxeParameterModel model = getUserData(HAXE_PARAMETER_MODEL_KEY);

This comment has been minimized.

@EricBishton

EricBishton Jun 21, 2018

Member

We're rewriting this function a lot. Maybe we could put it in a base class, so that we can call it like:

 return super.getModel(HAXE_PARAMETER_MODEL_KEY, 
               () -> new HaxeParameterModel((HaxeParameter)this);
PsiFileSystemItem current = to;
while (current != null) {
while (current != null && !current.equals(from)) {

This comment has been minimized.

@EricBishton

EricBishton Jun 21, 2018

Member

This is a significant change in logic. The old version of the function would return a single entry in the list if from and to were equal; current would be added to the items list if it matched from -- and then the loop would break. In the new code, that won't happen and the returned list will be empty. Was this change intentional, or were you refactoring this function to make it easier to read and missed that point?

This comment has been minimized.

@EricBishton

EricBishton Jun 21, 2018

Member

Or, are you just repurposing an no-longer-used API? Though I think the new code that uses this function still expects to get back a single item in the list.

@@ -285,4 +286,47 @@ public static PsiElement getNextSiblingSkipWhiteSpacesAndComments(@Nullable PsiE
return sibling;
}
@Nullable
public static PsiElement findSiblingForward(@NotNull final PsiElement element,

This comment has been minimized.

@EricBishton

EricBishton Jun 21, 2018

Member

Why not just use getNext/PrevSiblingSkippingCondition()? It seems to do the same thing (at least the way the current code is using it).

In these functions, I don't see the point of the consumer at all. It is called for elements that the functions aren't looking for. Elements which match are never presented to the consumer. What is the anticipated use of these functions?

@@ -2,7 +2,7 @@ package ;
class AbstractAssignmentFromTo1 {
public static function test():Void {
var <error descr="Incompatible type MyArray<Int> can't be assigned from Array<Int>">val:MyArray<Int> = [10]</error>;
var <error descr="<html><body>Incompatible types.<table><tr><td>Expected:</td><td><b>MyArray<Int></b></td></tr><tr><td>Found:</td><td><font color='red'><b>Array<Int></b></font></td></tr></table></body></html>">val:MyArray<Int> = [10]</error>;

This comment has been minimized.

@EricBishton

EricBishton Jun 21, 2018

Member

These results are hard to decipher. I wonder if a test mode that puts out the data without the HTML might be easier/better.

This comment has been minimized.

@EricBishton

EricBishton Jun 21, 2018

Member

Also, I'm not sure that the explanatory text is easier to understand. It displays the two types clearly, but doesn't tell you why the types are incompatible, while the old one does.

class Bar4 extends Bar5 {}
class Bar5 extends Bar4 {}
class Bar5 extends Bar4 {

This comment has been minimized.

@EricBishton

EricBishton Jun 21, 2018

Member

Is this illegal cycle actually detected? It should be. So should a longer cycle (e.g Bar5->Bar4->Bar3->Bar2->Bar5).

@@ -0,0 +1,12 @@
class Implementation implements <error descr="Not i">InterA {

This comment has been minimized.

@EricBishton

EricBishton Jun 21, 2018

Member

This looks incomplete. (No ending error tag). Is this used?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment