During refactoring in yang text editor highlights elements that should not be refactored

[kkarmakul]

Refactor doesn't take into account prefix of an idetifier:

I tried to rename a grouping defined locally, but the IDE thinks that imported "aggregate-flow-statistics" should be renamed also.

[davidmichaelkarr]

I've spent some time debugging this, and I see where something needs to be modified, but I haven't determined the exact code changes yet.

The basic problem area is in "com.cisco.yangide.ext.refactoring.actions.RenameSupport.findLocalReferences(Module, ASTNamedNode)":

    public static ASTNamedNode[] findLocalReferences(Module module, final ASTNamedNode node) {
        final List<ASTNamedNode> nodes = new ArrayList<>();
        final String name = node.getName();
        module.accept(new ASTVisitor() {
            @Override
            public void preVisit(ASTNode n) {
                if (n instanceof ASTNamedNode) {
                    ASTNamedNode nn = (ASTNamedNode) n;
                    if ((nn instanceof TypeDefinition || nn instanceof GroupingDefinition || nn instanceof Module
                            || nn instanceof SubModule || nn instanceof IdentitySchemaNode)
                            && nn.getName().equals(name)) {
                        nodes.add(nn);
                    } else if (nn instanceof TypeReference && ((TypeReference) nn).getType().getName().equals(name)) {
                        nodes.add(nn);
                    } else if (nn instanceof UsesNode && ((UsesNode) nn).getGrouping().getName().equals(name)) {
                        nodes.add(nn);
                    } else if (nn instanceof BaseReference && ((BaseReference) nn).getType().getName().equals(name)) {
                        nodes.add(nn);
                    } else if (nn instanceof ModuleImport && ((ModuleImport) nn).getName().equals(name)) {
                        nodes.add(nn);
                    } else if (nn instanceof SubModuleInclude && ((SubModuleInclude) nn).getName().equals(name)) {
                        nodes.add(nn);
                    }
                }
            }
        });
        return nodes.toArray(new ASTNamedNode[nodes.size()]);
    }
}

The problem is that only the "name" is compared. The QName has a prefix and a module, both of which will be different for those selections with the same name, but a different prefix.

The problem is, what can we compare those values against? The method comes in with a Module and a ASTNamedNode.

Is the idea that we should store the prefix of the module, in addition to the name of the node, and for each of those specific types, cast to the subtype that provides the prefix value (no simple mixins in Java, natch) and compare the prefix value in addition to the name value? Any subtle holes in that strategy?

[davidmichaelkarr]

Ok, the following at least addresses the simple case described in the issue:

    public static ASTNamedNode[] findLocalReferences(Module module, final ASTNamedNode node) {
        final List<ASTNamedNode> nodes = new ArrayList<>();
        final String                name            = node.getName();
        final SimpleNode<String>    modulePrefix    = module.getPrefix();
        module.accept(new ASTVisitor() {
            @Override
            public void preVisit(ASTNode n) {
                if (n instanceof ASTNamedNode) {
                    ASTNamedNode nn = (ASTNamedNode) n;
                    if ((nn instanceof TypeDefinition || nn instanceof GroupingDefinition || nn instanceof Module
                            || nn instanceof SubModule || nn instanceof IdentitySchemaNode)
                            && nn.getName().equals(name)) {
                        nodes.add(nn);
                    } else if (nn instanceof TypeReference && ((TypeReference) nn).getType().getName().equals(name)) {
                        nodes.add(nn);
                    } else if (nn instanceof UsesNode && ((UsesNode) nn).getGrouping().getName().equals(name)) {
                        // We add the node if the prefix on the UsesNode matches the module prefix, either
                        // implicitly or explicitly.
                        if (((UsesNode) nn).getGrouping().getPrefix() != null && modulePrefix != null &&
                            ((UsesNode) nn).getGrouping().getPrefix().equals(modulePrefix.getValue())) {
                            nodes.add(nn);
                        }
                    } else if (nn instanceof BaseReference && ((BaseReference) nn).getType().getName().equals(name)) {
                        nodes.add(nn);
                    } else if (nn instanceof ModuleImport && ((ModuleImport) nn).getName().equals(name)) {
                        nodes.add(nn);
                    } else if (nn instanceof SubModuleInclude && ((SubModuleInclude) nn).getName().equals(name)) {
                        nodes.add(nn);
                    }
                }
            }
        });
        return nodes.toArray(new ASTNamedNode[nodes.size()]);
    }

There are several things about this that I'm unsure of:

• Exactly what other node types will need similar treatment?
• Are there other combinations of the module prefix and node prefix that I need to check for?
• Is there any effective guarantee that either the grouping prefix or the module prefix will NEVER be null, just so I don't have to check that in each iteration?

I'd like to build a test to validate this functionality, but being as neither SWTBot or RCPTT has been implemented yet in this project, that will take some effort.

I suppose I'll file a PR for the change described above, but I understand it's possible it will never be merged.