From 269d35a6c193872f44ba278439efe906905047a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Galland?= Date: Thu, 15 Oct 2015 09:35:50 +0200 Subject: [PATCH] [lang] Add @Pure annotation on no-side-effect actions. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit see #191 Signed-off-by: Stéphane Galland --- .../src/io/sarl/lang/SARLRuntimeModule.java | 12 +- .../lang/jvmmodel/SARLJvmModelInferrer.java | 28 +-- .../typing/ExtendedXExpressionHelper.java | 72 +++++++ ...Helper.java => SARLXExpressionHelper.java} | 40 +--- .../lang/typing/XExpressionConstants.java | 47 +++++ .../compilation/general/PureFunctionTest.java | 183 ++++++++++++------ .../compilation/oop/ClassCompilerTest.java | 5 + 7 files changed, 283 insertions(+), 104 deletions(-) create mode 100644 plugins/io.sarl.lang/src/io/sarl/lang/typing/ExtendedXExpressionHelper.java rename plugins/io.sarl.lang/src/io/sarl/lang/typing/{SARLExpressionHelper.java => SARLXExpressionHelper.java} (54%) create mode 100644 plugins/io.sarl.lang/src/io/sarl/lang/typing/XExpressionConstants.java diff --git a/plugins/io.sarl.lang/src/io/sarl/lang/SARLRuntimeModule.java b/plugins/io.sarl.lang/src/io/sarl/lang/SARLRuntimeModule.java index 79e39c3964..6d9eb497b2 100644 --- a/plugins/io.sarl.lang/src/io/sarl/lang/SARLRuntimeModule.java +++ b/plugins/io.sarl.lang/src/io/sarl/lang/SARLRuntimeModule.java @@ -124,7 +124,8 @@ import io.sarl.lang.jvmmodel.SarlJvmModelAssociations; import io.sarl.lang.sarl.SarlFactory; import io.sarl.lang.scoping.batch.SARLImplicitlyImportedFeatures; -import io.sarl.lang.typing.SARLExpressionHelper; +import io.sarl.lang.typing.ExtendedXExpressionHelper; +import io.sarl.lang.typing.SARLXExpressionHelper; import io.sarl.lang.validation.DefaultFeatureCallValidator; import io.sarl.lang.validation.FeatureCallValidator; import io.sarl.lang.validation.SARLConfigurableIssueCodesProvider; @@ -323,7 +324,14 @@ public Class bindResourceChangeRegistry() { * @return the type of the XExpression helper. */ public Class bindXExpressionHelper() { - return SARLExpressionHelper.class; + return SARLXExpressionHelper.class; + } + + /** Replies the type of the extended elper for using XExpressions. + * @return the type of the XExpression helper. + */ + public Class bindExtendedXExpressionHelper() { + return ExtendedXExpressionHelper.class; } @Override diff --git a/plugins/io.sarl.lang/src/io/sarl/lang/jvmmodel/SARLJvmModelInferrer.java b/plugins/io.sarl.lang/src/io/sarl/lang/jvmmodel/SARLJvmModelInferrer.java index 3ba55e6260..59e2e75c44 100644 --- a/plugins/io.sarl.lang/src/io/sarl/lang/jvmmodel/SARLJvmModelInferrer.java +++ b/plugins/io.sarl.lang/src/io/sarl/lang/jvmmodel/SARLJvmModelInferrer.java @@ -116,7 +116,7 @@ import io.sarl.lang.sarl.SarlFormalParameter; import io.sarl.lang.sarl.SarlRequiredCapacity; import io.sarl.lang.sarl.SarlSkill; -import io.sarl.lang.typing.SARLExpressionHelper; +import io.sarl.lang.typing.ExtendedXExpressionHelper; import io.sarl.lang.util.JvmVisibilityComparator; import io.sarl.lang.util.Utils; @@ -160,6 +160,11 @@ public class SARLJvmModelInferrer extends XtendJvmModelInferrer { */ @Inject private CommonTypeComputationServices services; + + /** Extended helper for using XExpressions. + */ + @Inject + private ExtendedXExpressionHelper extendedExpressionHelper; /** JVM type services. */ @@ -934,6 +939,11 @@ protected void transform(final XtendFunction source, final JvmGenericType contai operation.getExceptions().add(this.typeBuilder.cloneWithProxies(exception)); } + // Create extension / Body + if (!operation.isAbstract() && !container.isInterface()) { + setBody(operation, expression); + } + // Annotations translateAnnotationsTo(source.getAnnotations(), operation); if (source.isOverride() @@ -941,14 +951,11 @@ protected void transform(final XtendFunction source, final JvmGenericType contai && this.typeReferences.findDeclaredType(Override.class, source) != null) { operation.getAnnotations().add(this._annotationTypesBuilder.annotationRef(Override.class)); } - if (!source.isAbstract() && - (expression == null - || !((SARLExpressionHelper) this.services.getExpressionHelper()).hasDeepSideEffects(expression))) { + if ((this.extendedExpressionHelper.isPureOperation(operation, expression)) + && (!Utils.hasAnnotation(operation, Pure.class)) + && (this.typeReferences.findDeclaredType(Pure.class, source) != null)) { // The function is pure - if (!Utils.hasAnnotation(operation, Pure.class) - && this.typeReferences.findDeclaredType(Pure.class, source) != null) { - operation.getAnnotations().add(this._annotationTypesBuilder.annotationRef(Pure.class)); - } + operation.getAnnotations().add(this._annotationTypesBuilder.annotationRef(Pure.class)); } // Detecting if the action is an early-exit action. @@ -969,11 +976,6 @@ protected void transform(final XtendFunction source, final JvmGenericType contai operation.getAnnotations().add(annotationClassRef(FiredEvent.class, firedEvents)); } - // Create extension / Body - if (!operation.isAbstract() && !container.isInterface()) { - setBody(operation, expression); - } - // 1. Ensure that the Java annotations related to the default value are really present. // They may be not present if the generated action is a specific version of an inherited // action with default values for parameters. diff --git a/plugins/io.sarl.lang/src/io/sarl/lang/typing/ExtendedXExpressionHelper.java b/plugins/io.sarl.lang/src/io/sarl/lang/typing/ExtendedXExpressionHelper.java new file mode 100644 index 0000000000..4091b1a795 --- /dev/null +++ b/plugins/io.sarl.lang/src/io/sarl/lang/typing/ExtendedXExpressionHelper.java @@ -0,0 +1,72 @@ +/* + * $Id$ + * + * SARL is an general-purpose agent programming language. + * More details on http://www.sarl.io + * + * Copyright (C) 2014-2015 the original authors or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.sarl.lang.typing; + +import java.util.regex.Pattern; + +import com.google.inject.Inject; +import com.google.inject.Singleton; +import org.eclipse.xtext.common.types.JvmOperation; +import org.eclipse.xtext.xbase.XExpression; +import org.eclipse.xtext.xbase.lib.Pure; +import org.eclipse.xtext.xbase.util.XExpressionHelper; + +/** + * Extended helper on expressions. + * + * @author $Author: sgalland$ + * @version $FullVersion$ + * @mavengroupid $GroupId$ + * @mavenartifactid $ArtifactId$ + * @see SARLXExpressionHelper + */ +@Singleton +public class ExtendedXExpressionHelper { + + @Inject + private XExpressionHelper expressionHelper; + + private final Pattern pattern; + + /** Construct the helper. + */ + public ExtendedXExpressionHelper() { + this.pattern = Pattern.compile(XExpressionConstants.SPECIAL_PURE_FUNCTION_NAME_PATTERN); + } + + /** Check if the given operation could be annoted with "@Pure". + * + * @param operation - the operation to test. + * @param body - the body of the operation. + * @return true if one of the components of the given expression has a side effect; + * otherwise false. + * @see Pure + */ + public boolean isPureOperation(JvmOperation operation, XExpression body) { + if (operation == null || operation.isAbstract() || body == null) { + return false; + } + String name = operation.getSimpleName(); + return (name != null && this.pattern.matcher(name).find()) || !this.expressionHelper.hasSideEffects(body); + } + +} diff --git a/plugins/io.sarl.lang/src/io/sarl/lang/typing/SARLExpressionHelper.java b/plugins/io.sarl.lang/src/io/sarl/lang/typing/SARLXExpressionHelper.java similarity index 54% rename from plugins/io.sarl.lang/src/io/sarl/lang/typing/SARLExpressionHelper.java rename to plugins/io.sarl.lang/src/io/sarl/lang/typing/SARLXExpressionHelper.java index 7839b3a35b..7a0f858a81 100644 --- a/plugins/io.sarl.lang/src/io/sarl/lang/typing/SARLExpressionHelper.java +++ b/plugins/io.sarl.lang/src/io/sarl/lang/typing/SARLXExpressionHelper.java @@ -23,9 +23,6 @@ import java.util.regex.Pattern; -import org.eclipse.xtext.xbase.XExpression; - -import org.eclipse.xtext.xbase.XBlockExpression; import com.google.inject.Singleton; import org.eclipse.xtend.core.typing.XtendExpressionHelper; import org.eclipse.xtext.common.types.JvmIdentifiableElement; @@ -35,6 +32,10 @@ /** * Helper on expressions. * + *

This implementation extends the Xtend expression helper by assuming that any function + * with a name starting with "get", "is", "has" is a pure function. + * It also assumes that "equals", "hashCode", "clone" and "toString" are also pure functions. + * * @author $Author: sgalland$ * @version $FullVersion$ * @mavengroupid $GroupId$ @@ -42,14 +43,14 @@ * @see "http://www.eclipse.org/Xtext/documentation.html#validation" */ @Singleton -public class SARLExpressionHelper extends XtendExpressionHelper { +public class SARLXExpressionHelper extends XtendExpressionHelper { private final Pattern pattern; /** Construct the helper. */ - public SARLExpressionHelper() { - this.pattern = Pattern.compile("^(((is)|(get)|(has))[A-Z].*)|(equals)|(hashCode)|(clone)|(toString)$"); //$NON-NLS-1$ + public SARLXExpressionHelper() { + this.pattern = Pattern.compile(XExpressionConstants.SPECIAL_PURE_FUNCTION_NAME_PATTERN); } @Override @@ -67,31 +68,4 @@ public boolean hasSideEffects(XAbstractFeatureCall featureCall, boolean inspectC return false; } - /** Replies if the given expression may the root expression for a pure function. - * - *

A pure function causes no externally visible side-effects and does not mutate non-local state. - * In other words: if the result of a pure function is not used, it is dead code and is supposed to - * be removeable without changing the behavior of the program. - * - *

This function extends the function {@link #hasSideEffects(org.eclipse.xtext.xbase.XExpression)} - * by assuming that {@link XBlockExpression} expressions do not cause side effects if its member - * expressions do not cause side effects. - * - *

This function is a recursive version of {@link #hasSideEffects(org.eclipse.xtext.xbase.XExpression)}. - * - * @param expression - the expression to test. - * @return whether the expression itself (not its children) possibly causes a side-effect - */ - public boolean hasDeepSideEffects(XExpression expression) { -// if (expression instanceof XBlockExpression) { -// for (XExpression subexpression : ((XBlockExpression) expression).getExpressions()) { -// if (hasDeepSideEffects(subexpression)) { -// return true; -// } -// } -// return false; -// } - return hasSideEffects(expression); - } - } diff --git a/plugins/io.sarl.lang/src/io/sarl/lang/typing/XExpressionConstants.java b/plugins/io.sarl.lang/src/io/sarl/lang/typing/XExpressionConstants.java new file mode 100644 index 0000000000..a47c08be13 --- /dev/null +++ b/plugins/io.sarl.lang/src/io/sarl/lang/typing/XExpressionConstants.java @@ -0,0 +1,47 @@ +/* + * $Id$ + * + * SARL is an general-purpose agent programming language. + * More details on http://www.sarl.io + * + * Copyright (C) 2014-2015 the original authors or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.sarl.lang.typing; + +import com.google.inject.Singleton; + +/** + * Constants used by the XExpression helpers. + * + * @author $Author: sgalland$ + * @version $FullVersion$ + * @mavengroupid $GroupId$ + * @mavenartifactid $ArtifactId$ + * @see SARLXExpressionHelper + */ +@Singleton +public final class XExpressionConstants { + + /** Regular expression pattern that matches the names of functions usually + * considered as pure. + */ + public static final String SPECIAL_PURE_FUNCTION_NAME_PATTERN = "^(((is)|(get)|(has))[A-Z].*)|(equals)|(hashCode)|(clone)|(toString)$"; //$NON-NLS-1$; + + private XExpressionConstants() { + // + } + +} diff --git a/tests/io.sarl.lang.tests/src/io/sarl/lang/tests/compilation/general/PureFunctionTest.java b/tests/io.sarl.lang.tests/src/io/sarl/lang/tests/compilation/general/PureFunctionTest.java index cfbb0dfdc9..3ee20b1ffd 100644 --- a/tests/io.sarl.lang.tests/src/io/sarl/lang/tests/compilation/general/PureFunctionTest.java +++ b/tests/io.sarl.lang.tests/src/io/sarl/lang/tests/compilation/general/PureFunctionTest.java @@ -50,7 +50,6 @@ @RunWith(Suite.class) @SuiteClasses({ PureFunctionTest.DefinitionTests.class, - PureFunctionTest.UseTests.class, }) @SuppressWarnings("all") public class PureFunctionTest { @@ -101,7 +100,7 @@ public void accept(Result r) { } @Test - public void noPureParent_pureLocal() throws Exception { + public void noPureParent_pureLocal_tooComplexToBeDetected() throws Exception { String source = multilineString( "class C1 {", " def fct { return Math.random }", @@ -120,12 +119,9 @@ public void noPureParent_pureLocal() throws Exception { "" ); final String expectedC2 = multilineString( - "import org.eclipse.xtext.xbase.lib.Pure;", - "", "@SuppressWarnings(\"all\")", "public class C2 extends C1 {", " @Override", - " @Pure", " public double fct() {", " return 0;", " }", @@ -142,7 +138,7 @@ public void accept(Result r) { } @Test - public void pureParent_pureLocal() throws Exception { + public void pureParent_pureLocal_tooComplexToBeDetected() throws Exception { String source = multilineString( "class C1 {", " def fct { }", @@ -152,23 +148,17 @@ public void pureParent_pureLocal() throws Exception { "}", ""); final String expectedC1 = multilineString( - "import org.eclipse.xtext.xbase.lib.Pure;", - "", "@SuppressWarnings(\"all\")", "public class C1 {", - " @Pure", " public void fct() {", " }", "}", "" ); final String expectedC2 = multilineString( - "import org.eclipse.xtext.xbase.lib.Pure;", - "", "@SuppressWarnings(\"all\")", "public class C2 extends C1 {", " @Override", - " @Pure", " public void fct() {", " }", "}", @@ -222,7 +212,7 @@ public void accept(Result r) { } @Test - public void abstractParent_pureLocal() throws Exception { + public void abstractParent_pureLocal_tooComplexToBeDetected() throws Exception { String source = multilineString( "abstract class C1 {", " abstract def fct : double", @@ -239,12 +229,9 @@ public void abstractParent_pureLocal() throws Exception { "" ); final String expectedC2 = multilineString( - "import org.eclipse.xtext.xbase.lib.Pure;", - "", "@SuppressWarnings(\"all\")", "public class C2 extends C1 {", " @Override", - " @Pure", " public double fct() {", " return 0;", " }", @@ -260,52 +247,136 @@ public void accept(Result r) { }); } - } + @Test + public void special_get() throws Exception { + String source = multilineString( + "class C1 {", + " def getXXX : double { 9 }", + "}", + ""); + final String expectedC1 = multilineString( + "import org.eclipse.xtext.xbase.lib.Pure;", + "", + "@SuppressWarnings(\"all\")", + "public class C1 {", + " @Pure", + " public double getXXX() {", + " return 9;", + " }", + "}", + "" + ); + this.compiler.assertCompilesTo(source, expectedC1); + } + + @Test + public void special_is() throws Exception { + String source = multilineString( + "class C1 {", + " def isXXX : boolean { false }", + "}", + ""); + final String expectedC1 = multilineString( + "import org.eclipse.xtext.xbase.lib.Pure;", + "", + "@SuppressWarnings(\"all\")", + "public class C1 {", + " @Pure", + " public boolean isXXX() {", + " return false;", + " }", + "}", + "" + ); + this.compiler.assertCompilesTo(source, expectedC1); + } + + @Test + public void special_has() throws Exception { + String source = multilineString( + "class C1 {", + " def hasXXX : boolean { false }", + "}", + ""); + final String expectedC1 = multilineString( + "import org.eclipse.xtext.xbase.lib.Pure;", + "", + "@SuppressWarnings(\"all\")", + "public class C1 {", + " @Pure", + " public boolean hasXXX() {", + " return false;", + " }", + "}", + "" + ); + this.compiler.assertCompilesTo(source, expectedC1); + } - public static class UseTests extends AbstractSarlTest { + @Test + public void special_toString() throws Exception { + String source = multilineString( + "class C1 {", + " def toString : String { \"\" }", + "}", + ""); + final String expectedC1 = multilineString( + "import org.eclipse.xtext.xbase.lib.Pure;", + "", + "@SuppressWarnings(\"all\")", + "public class C1 {", + " @Pure", + " public String toString() {", + " return \"\";", + " }", + "}", + "" + ); + this.compiler.assertCompilesTo(source, expectedC1); + } @Test - public void invalidUsageOfPureFunction() throws Exception { - SarlScript mas = file(multilineString( - "behavior B1 {", - " var attr = 5", - " def purefct : int { 5 }", - "}", - "behavior B2 {", - " var inst = new B1", - " def testfct {", - " inst.purefct", - " }", - "}", - "")); - validate(mas).assertError( - SarlPackage.eINSTANCE.getSarlBehavior(), - org.eclipse.xtend.core.validation.IssueCodes.CLASS_EXPECTED, - 36, 2, - "Invalid supertype. Expecting a class"); + public void special_hashCode() throws Exception { + String source = multilineString( + "class C1 {", + " def hashCode : int { 0 }", + "}", + ""); + final String expectedC1 = multilineString( + "import org.eclipse.xtext.xbase.lib.Pure;", + "", + "@SuppressWarnings(\"all\")", + "public class C1 {", + " @Pure", + " public int hashCode() {", + " return 0;", + " }", + "}", + "" + ); + this.compiler.assertCompilesTo(source, expectedC1); } @Test - public void validUsageOfPureFunction() throws Exception { - SarlScript mas = file(multilineString( - "behavior B1 {", - " new () { super(null) }", - " var attr = 5", - " def purefct : int { 5 }", - "}", - "behavior B2 {", - " new () { super(null) }", - " var inst = new B1", - " def testfct : int {", - " inst.purefct", - " }", - "}", - "")); - validate(mas).assertError( - SarlPackage.eINSTANCE.getSarlBehavior(), - org.eclipse.xtend.core.validation.IssueCodes.CLASS_EXPECTED, - 36, 2, - "Invalid supertype. Expecting a class"); + public void special_equals() throws Exception { + String source = multilineString( + "class C1 {", + " def equals(a : Object) : boolean { false }", + "}", + ""); + final String expectedC1 = multilineString( + "import org.eclipse.xtext.xbase.lib.Pure;", + "", + "@SuppressWarnings(\"all\")", + "public class C1 {", + " @Pure", + " public boolean equals(final Object a) {", + " return false;", + " }", + "}", + "" + ); + this.compiler.assertCompilesTo(source, expectedC1); } } diff --git a/tests/io.sarl.lang.tests/src/io/sarl/lang/tests/compilation/oop/ClassCompilerTest.java b/tests/io.sarl.lang.tests/src/io/sarl/lang/tests/compilation/oop/ClassCompilerTest.java index f6e069ef26..fb906378d5 100644 --- a/tests/io.sarl.lang.tests/src/io/sarl/lang/tests/compilation/oop/ClassCompilerTest.java +++ b/tests/io.sarl.lang.tests/src/io/sarl/lang/tests/compilation/oop/ClassCompilerTest.java @@ -186,12 +186,15 @@ public void methodOverriding_explicitReturnType() throws Exception { final String expectedPerson = multilineString( "package io.sarl.docs.reference.oop;", "", + "import org.eclipse.xtext.xbase.lib.Pure;", + "", "@SuppressWarnings(\"all\")", "public class Person {", " private String firstName;", " ", " private String lastName;", " ", + " @Pure", " public String getFullName() {", " return ((this.firstName + \" \") + this.lastName);", " }", @@ -201,12 +204,14 @@ public void methodOverriding_explicitReturnType() throws Exception { "package io.sarl.docs.reference.oop;", "", "import io.sarl.docs.reference.oop.Person;", + "import org.eclipse.xtext.xbase.lib.Pure;", "", "@SuppressWarnings(\"all\")", "public class PersonEx extends Person {", " private String title;", " ", " @Override", + " @Pure", " public String getFullName() {", " String _fullName = super.getFullName();", " return ((this.title + \" \") + _fullName);",