Skip to content
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

Fix #372: addGlobalImport(Importer) is syntactic #382

Merged
merged 5 commits into from
Sep 29, 2017

Conversation

olafurpg
Copy link
Contributor

Review @abeln

Patch.apply assumed all TreePatches are semantic patches, which is
false. Some patches such as TreePatch.AddGlobalImport are syntactic.
PatchOps already guards against constructing semantic patches inside
syntactic rules, so there is no need to guard against it inside the
`private[scalafix] Patch.apply` method.
Added a docstring to clarify the rule is the default one used for tests.
@abeln
Copy link
Contributor

abeln commented Sep 29, 2017

LGTM. Thanks for fixing this!
Is there a reason why we can't use the type system to prevent this type of error (e.g. remove the semanticIndex option from Ctx)? Is it related to the fact that we want to be able to combine syntactic and semantic patches?

@olafurpg
Copy link
Contributor Author

Is there a reason why we can't use the type system to prevent this type of error

The implicit SemanticdbIndex parameter on ctx.addGlobalImport is used to statically guard against using semantic from syntactic rules. The index is not used in the method implementation however, it's not included as a field in the underlying Patch adt for several reasons

  • then we need to validate that all of the Patches have the same index
  • we only need the index to "interpret" the Patches into diffs, not when building them

I admit I'm still not very happy with the current design. I feel like there should be a much cleaner way to model things.

remove the semanticIndex option from Ctx

Can you elaborate?

Is it related to the fact that we want to be able to combine syntactic and semantic patches?

A lot of the complications in the scalafix api design are caused by the desire to abstract over syntactic/semantic rules. The fact that the semantic API has gone through several revisions has not helped either, but I suspect it will be stable onwards.

@olafurpg olafurpg merged commit 9c53a52 into scalacenter:master Sep 29, 2017
@olafurpg olafurpg deleted the add-global-import branch September 29, 2017 19:22
@olafurpg olafurpg added this to the v0.5.3 milestone Oct 11, 2017
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.

2 participants