Skip to content

Strongly typed AST: dictionary and set literal#294

Merged
andrea-guarino-sonarsource merged 3 commits intomasterfrom
dictionary-literals
Sep 4, 2019
Merged

Strongly typed AST: dictionary and set literal#294
andrea-guarino-sonarsource merged 3 commits intomasterfrom
dictionary-literals

Conversation

@andrea-guarino-sonarsource
Copy link
Copy Markdown
Contributor

No description provided.

@andrea-guarino-sonarsource andrea-guarino-sonarsource changed the title Strongly typed AST: dictionary literal Strongly typed AST: dictionary and set literal Sep 4, 2019
import java.util.Set;

public interface PySetLiteralTree extends PyDictOrSetLiteralTree {
Set<PyExpressionTree> elements();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a set ?


@Override
public void visitSetLiteral(PySetLiteralTree pySetLiteralTree) {
scan(new ArrayList<>(pySetLiteralTree.elements()));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to instantiate a new list if you are not using a set in the interface.

}

@Override
public Set<PyKeyValuePairTree> elements() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A set seems to be a not so good choice in terms of API : order is not guaranteed for API user whereas in the case of an AST order of elements matters.

import com.sonar.sslr.api.Token;
import java.util.List;

public interface PyDictOrSetLiteralTree extends PyExpressionTree {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this interface is just a way to factorize code (and btw, factorization by inheritance is not necessarily a good pattern) I believe you could push things one step further with a type parameter :

class PyDictOrSetLiteralTree<T ? extends ExpressionTree>...  {
  List<T> elements();
}

and specialize this in the inheritance.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure this is worth it though... I would eventually be more in favor of not complexifying the type hierarchy and I would strongly suggest to avoid this intermediate level in the API as it is not meaningful in the interfaces (those don't share a common thing in term of language construction). (fine in the implementation though, even if it is factorization through inheritance).


@Override
public List<Tree> children() {
return elements.stream().map(element -> (Tree) element).collect(Collectors.toList());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Collections.unmodifiableList would not work like in PyStatementListTreeImpl ?


@Override
public List<Tree> children() {
return elements.stream().map(element -> (Tree) element).collect(Collectors.toList());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Collections.unmodifiableList would not work like in PyStatementListTreeImpl ?

@benzonico benzonico self-requested a review September 4, 2019 14:35
@andrea-guarino-sonarsource andrea-guarino-sonarsource merged commit afb22e8 into master Sep 4, 2019
@andrea-guarino-sonarsource andrea-guarino-sonarsource deleted the dictionary-literals branch September 4, 2019 14:54
hashicorp-vault-sonar-prod Bot pushed a commit that referenced this pull request May 30, 2025
GitOrigin-RevId: aa295aedc41cb4da318c6a4d9e21ede340222a1c
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.

3 participants