Feature/refactor fix more tests #173

kindlich · 2024-09-06T21:34:53Z

No description provided.

Currently it still fails, though.

…ded by an expansion with generic arguments

… for Definitions shadowing globals

kindlich · 2024-09-07T22:24:41Z

CodeModel/src/main/java/org/openzen/zenscript/codemodel/FunctionHeader.java

@@ -469,4 +470,8 @@ public int hashCode() {
 		result = 31 * result + (hasUnknowns ? 1 : 0);
 		return result;
 	}
+
+	public boolean hasInvalidTypes() {
+		return Stream.of(parameters).anyMatch(p -> p.type.isInvalid());


💭 Thought: While writing this I found out we have BasicTypeID.INVALID and InvalidTypeID.
Do we need to change some calls that only compare for == BasicTypeID.INVALID to check for isInvalid() instead?

Yes, we should change that.

kindlich · 2024-09-07T22:26:35Z

CodeModel/src/main/java/org/openzen/zenscript/codemodel/compilation/ResolvedType.java

@@ -27,13 +28,19 @@ public interface ResolvedType {
 	default Optional<Expression> tryCastExplicit(TypeID target, ExpressionCompiler compiler, CodePosition position, Expression value, boolean optional) {
 		return findCaster(target)
 				.filter(caster -> !caster.getModifiers().isImplicit())
-				.map(caster -> caster.call(compiler.at(position), value, CallArguments.EMPTY));
+				.flatMap(caster -> MatchedCallArguments.match(compiler, position, Collections.singletonList(caster), target, TypeID.NONE)


These changes were done since we need the ExpansionTypeParameters even for a caster.
Is there a better way than matching the method like this or is this fine?

kindlich · 2024-09-07T22:26:54Z

...n/java/org/openzen/zenscript/codemodel/compilation/impl/compiler/ExpressionCompilerImpl.java

-		Optional<IGlobal> global = context.findGlobal(name.name);
-		if (global.isPresent())
-			return global.map(g -> g.getExpression(position, name.arguments).compile(this));
+		// Resolve a local type first so that functions can shadow Globals


Does this have any side-effects that could cause us trouble?

kindlich · 2024-09-07T22:27:38Z

CodeModel/src/main/java/org/openzen/zenscript/codemodel/statement/BlockStatement.java

@@ -74,6 +75,10 @@ public Statement transform(ExpressionTransformer transformer, ConcatMap<LoopStat

 	@Override
 	public Optional<TypeID> getReturnType() {
+		if(statements.length == 0) {


I wasn't sure if this check should be a special-case check here or further below if the collect List is empty?

kindlich · 2024-09-07T22:30:52Z

...java/org/openzen/zenscript/javabytecode/compiler/definitions/JavaExpansionMemberVisitor.java


-		int i = 1;
+		int i = 0;


💭 Thought: We probably need to do similar changes to the other methods in this class, but so far only this one caused Tests to fail 🤷🏽‍♂️

kindlich · 2024-09-07T22:31:58Z

Parser/src/main/java/org/openzen/zenscript/parser/expression/ParsedExpressionArray.java

-				return resolvedType.findImplicitConstructor()
-						.map(constructor -> constructor.casted(compiler, position, cast, null, this))
-						.orElseGet(() -> cast.invalid(CompileErrors.invalidArrayType(cast.type)));
+				Optional<CastedExpression> implicitConstructor = resolvedType.findImplicitConstructor()


In theory this and check and the cast.of(eval()) branch cause implicit ctors to be checked twice.
But if we remove this here, some matching didn't work so I left it in

kindlich · 2024-09-07T22:34:16Z

Parser/src/main/java/org/openzen/zenscript/parser/member/ParsedConstructor.java

-			compiled.setBody(body.compile(compiler.forMethod(compiled.header.withReturnType(BasicTypeID.VOID))));
+
+			// Implicit .ctors return the created object, but "normal" ones don't, so for non-implict .ctors we change the header to return VOID
+			FunctionHeader compilingHeader = modifiers.isImplicit() ? compiled.header : compiled.header.withReturnType(BasicTypeID.VOID);


❓ Question: Is this the proper place for that check?

kindlich · 2024-09-14T12:53:47Z

CodeModel/src/main/java/org/openzen/zenscript/codemodel/compilation/CompileErrors.java

@@ -559,4 +567,12 @@ public static CompileError invalidPropertySetter(TypeID type, String name) {
 	public static CompileError invalidPropertyPair(TypeID type, String name) {
 		return new CompileError(CompileExceptionCode.INVALID_PROPERTY_PAIR, "Setter and getter of property " + name  + " in type " + type + " have different types");
 	}
+
+	public static CompileError invalidLambdaHeader(FunctionHeader header) {


🏷️ Naming: We probably need a better name here

kindlich · 2024-09-14T12:54:14Z

ScriptingEngineTester/src/main/resources/zencode_tests/classes/return_type.zc

@@ -1,4 +1,5 @@
-#error: 4:TYPE_NOT_INFERRED
+#error: 5:TYPE_NOT_INFERRED
+#error: 5:MISSING_RETURN


❓ Question: Should this report both errors?

kindlich · 2024-09-14T12:55:46Z

Validator/src/main/java/org/openzen/zenscript/validator/visitors/ReturnStatementValidator.java

+	 * @param statement  the statement to validate
+	 * @param validator  the validator used to log errors
+	 */
+	public static void validate(TypeID returnType, Statement statement, Validator validator) {


❓ Are all members checked by now?

FunctionalMembers (methods, operators, ...)

Getters

Implicit constructors (only implicit)

FunctionsDefinition

setters also, with return type void

kindlich · 2024-09-14T12:56:48Z

Validator/src/main/java/org/openzen/zenscript/validator/visitors/ReturnStatementValidator.java

+	 * @param validator  the validator used to log errors
+	 */
+	public static void validate(TypeID returnType, Statement statement, Validator validator) {
+		if (returnType == BasicTypeID.VOID) {


💭 Discussion: We could move the returnType check outside this function and only ever call this function if we know the type is not void. Which approach do we like more?

kindlich · 2024-09-14T12:57:20Z

Validator/src/main/java/org/openzen/zenscript/validator/visitors/ReturnStatementValidator.java

+				.map(it -> it.content.accept(this))
+				.reduce(ReturnKind::branches)
+				.orElse(ReturnKind.NEVER_RETURNS);
+		ReturnKind finallyClause = statement.finallyClause == null ? ReturnKind.NEVER_RETURNS : statement.finallyClause.accept(this);


❓ Question: Do we actually allow return statements inside finally blocks?

kindlich · 2024-09-14T12:57:56Z

Validator/src/main/java/org/openzen/zenscript/validator/visitors/ReturnStatementValidator.java

+		for (Statement statement : statements) {
+			if (result == ReturnKind.ALWAYS_RETURNS) {
+				validator.logError(statement.position, CompileErrors.unreachableStatement());
+				break;


❓ Question: Should we log this for all following statements or is the first one fine?

kindlich · 2024-09-14T12:59:28Z

Validator/src/main/java/org/openzen/zenscript/validator/visitors/ReturnStatementValidator.java

+		public static ReturnKind branches(ReturnKind a, ReturnKind b) {
+			if (a == NEVER_RETURNS && b == NEVER_RETURNS) {
+				return NEVER_RETURNS;
+			}
+			if (a == ALWAYS_RETURNS && b == ALWAYS_RETURNS) {
+				return ALWAYS_RETURNS;
+			}
+			return RETURNS_IN_SOME_CASES;
+		}


💭 Discussion: The same functionality would be achieved by the code block below.
I find the explicit if/else structure more readable, what do you think?

Suggested change

public static ReturnKind branches(ReturnKind a, ReturnKind b) {

if (a == NEVER_RETURNS && b == NEVER_RETURNS) {

return NEVER_RETURNS;

}

if (a == ALWAYS_RETURNS && b == ALWAYS_RETURNS) {

return ALWAYS_RETURNS;

}

return RETURNS_IN_SOME_CASES;

}

public static ReturnKind branches(ReturnKind a, ReturnKind b) {

return a == b ? a : RETURNS_IN_SOME_CASES;

}

stanhebben · 2024-10-04T18:20:49Z

CodeModel/src/main/java/org/openzen/zenscript/codemodel/FunctionHeader.java

@@ -469,4 +470,8 @@ public int hashCode() {
 		result = 31 * result + (hasUnknowns ? 1 : 0);
 		return result;
 	}
+
+	public boolean hasInvalidTypes() {
+		return Stream.of(parameters).anyMatch(p -> p.type.isInvalid());


Yes, we should change that.

stanhebben · 2024-10-04T18:39:41Z

Validator/src/main/java/org/openzen/zenscript/validator/visitors/ReturnStatementValidator.java

+
+	@Override
+	public ReturnKind visitForeach(ForeachStatement statement) {
+		return statement.getContent().accept(this);


Not correct if there are zero elements

stanhebben · 2024-10-04T19:02:49Z

Validator/src/main/java/org/openzen/zenscript/validator/visitors/ReturnStatementValidator.java

+	 * @param statement  the statement to validate
+	 * @param validator  the validator used to log errors
+	 */
+	public static void validate(TypeID returnType, Statement statement, Validator validator) {


setters also, with return type void

Fix implicit constructors not compiling properly

5635594

kindlich changed the base branch from development to feature/refactor September 6, 2024 21:35

kindlich added 8 commits September 6, 2024 23:43

Update shadowing test so that it actually tests what it is supposed to

1e881f2

Currently it still fails, though.

Fix: Also evaluate casters from arrays when casting an array-expression

58fb17c

tests: add some more tests

c9ad2f2

Fix compilation errors in trying to compile the an implicit caster ad…

f54672b

…ded by an expansion with generic arguments

fix: resolve types before globals when compiling variables to account…

4f38e49

… for Definitions shadowing globals

Fix ClassCastException trying to cast to InvalidExpression

e9786a9

Have empty function bodies evaluate as VOID return type

a510bcf

Cleanup: Change back name of variable

9079f02

kindlich commented Sep 7, 2024

View reviewed changes

kindlich added 2 commits September 14, 2024 14:50

Validate return statements inside functions and functional members

eeb4ebb

Report any statements after return statements as unreachable

55af6be

kindlich force-pushed the feature/refactor-fix-more-tests branch from 78dc709 to 55af6be Compare September 14, 2024 13:01

Validate return statements inside implicit constructors and setters

1ccc707

kindlich commented Sep 14, 2024

View reviewed changes

Fix array_map_key_values_1.zc

5f2f8a3

kindlich mentioned this pull request Sep 14, 2024

Feature/refactor capturing #174

Merged

kindlich marked this pull request as ready for review October 3, 2024 14:56

PR fixes and feedback

fe4b976

stanhebben approved these changes Oct 4, 2024

View reviewed changes

stanhebben merged commit 75ba801 into feature/refactor Oct 4, 2024
0 of 2 checks passed

kindlich mentioned this pull request Oct 4, 2024

Fix: Use isInvalid method instead of comparing to INVALID to check for invalidtypes #177

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/refactor fix more tests #173

Feature/refactor fix more tests #173

kindlich commented Sep 6, 2024

kindlich Sep 7, 2024

stanhebben Oct 4, 2024

kindlich Sep 7, 2024

kindlich Sep 7, 2024

kindlich Sep 7, 2024

kindlich Sep 7, 2024

kindlich Sep 7, 2024

kindlich Sep 7, 2024

kindlich Sep 14, 2024

kindlich Sep 14, 2024

kindlich Sep 14, 2024

stanhebben Oct 4, 2024

kindlich Sep 14, 2024

kindlich Sep 14, 2024

kindlich Sep 14, 2024

kindlich Sep 14, 2024

stanhebben Oct 4, 2024

stanhebben Oct 4, 2024

stanhebben Oct 4, 2024

Feature/refactor fix more tests #173

Feature/refactor fix more tests #173

Conversation

kindlich commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment