Adjust `polymorphicInstance()` #14635

cameel · 2023-10-23T16:03:29Z

The PR addresses #14510 (comment) and #14510 (comment).

We discussed this last week on the call - unifyGeneralized() was a part of some idea that was not fully realized in the end and all we really need is just a call to TypeEnvironment::fresh(). That's what polymorphicInstance() does now.

The PR also adds some tests covering the uses of fresh() and polymorphicInstances(). This shows that the PR does not break things both here and with my subsequent changes in #14606.

test/libsolidity/syntaxTests/experimental/inference/polymorphic_function_call_via_variable.sol

ekpyron · 2023-10-23T16:39:14Z

The funny thing is: the last thing I did before pushing the prototype was to do the exact opposite :-) - i.e. to introduce polymorphicInstance in the first place in order to isolate the occurrances of fresh() to make it easier to replace them by a mechanism accounting for #14606

cameel · 2023-10-23T17:08:48Z

Why would it be any easier to replace polymorphicInstance() than to replace fresh()?

ekpyron · 2023-10-23T17:11:53Z

Why would it be any easier to replace polymorphicInstance() than to replace fresh()?

The point was to collect all instances of refreshening for the purpose of generating a polymorphic schema of a global definition and then checking what's left.

Also: producing a freshened type is a technical operation on the type - producing a polymorphic instance of a global type scheme is a semantic description of what's happening.

ekpyron · 2023-10-23T17:12:38Z

Also: producing a freshened type is a technical operation on the type - producing a polymorphic instance of a global type scheme is a semantic description of what's happening.

Based on that I'd actually not remove it. (Removing the entire function that gives the operation that happens here a descriptive name is quite different from removing the additional type variable it unnecessarily introduces :-))

cameel · 2023-10-27T15:05:31Z

polymorphicInstance() restored.

cameel · 2023-10-30T12:16:45Z

I need reapproval. Only pushed a small stylistic tweak.

ekpyron · 2023-10-30T12:19:51Z

Before we merge new changes, we first need to clean up the mess in the base branch.

…ance()`

cameel · 2023-11-01T13:22:57Z

#14570 (comment)

It may actually make sense to just rename polymorphicInstance to generalize.

I wanted to merge this already, but in that case maybe I should still change it before I do?

ekpyron · 2023-11-01T13:46:37Z

I wanted to merge this already, but in that case maybe I should still change it before I do?

No hurry, just bringing up the option.

ekpyron · 2023-11-01T13:48:32Z

libsolidity/experimental/analysis/TypeInference.cpp

@@ -481,7 +480,10 @@ experimental::Type TypeInference::handleIdentifierByReferencedDeclaration(langut
 		else if (dynamic_cast<FunctionDefinition const*>(&_declaration))
 			return polymorphicInstance(*declarationAnnotation.type);
 		else if (dynamic_cast<TypeClassDefinition const*>(&_declaration))
-			return polymorphicInstance(*declarationAnnotation.type);
+		{
+			solAssert(TypeEnvironmentHelpers{*m_env}.typeVars(*declarationAnnotation.type).empty());


Wait, what? That doesn't seem right - shouldn't there be exactly one variable in there?

Ah, no, this is term context, so the use of the type class name as a special concrete type with its functions attached as members... I hope the distinction will become clearer after the upcoming refactors, we'll see.

So fine with merging the PR?

…ic-instance Adjust `polymorphicInstance()`

cameel added the experimental label Oct 23, 2023

cameel requested a review from ekpyron October 23, 2023 16:03

cameel self-assigned this Oct 23, 2023

cameel mentioned this pull request Oct 23, 2023

Type inference experiments. #14510

Merged

cameel commented Oct 23, 2023

View reviewed changes

test/libsolidity/syntaxTests/experimental/inference/polymorphic_function_call_via_variable.sol Outdated Show resolved Hide resolved

cameel requested a review from matheusaaguiar October 23, 2023 16:10

cameel mentioned this pull request Oct 23, 2023

Fixed type variables in function definitions in experimental analysis #14606

Closed

cameel force-pushed the new-analysis-remove-polymorphic-instance branch from 58e6b99 to 2ab295f Compare October 27, 2023 15:03

cameel changed the title ~~Replace polymorphicInstance() with fresh()~~ Adjust polymorphicInstance() Oct 27, 2023

ekpyron approved these changes Oct 30, 2023

View reviewed changes

cameel force-pushed the new-analysis-remove-polymorphic-instance branch from 2ab295f to 781a73b Compare October 30, 2023 11:52

cameel mentioned this pull request Oct 30, 2023

Use type class as a sort for its type variable #14654

Merged

cameel requested a review from ekpyron October 30, 2023 12:16

cameel force-pushed the new-analysis-remove-polymorphic-instance branch 2 times, most recently from a9bf87f to bc86754 Compare October 30, 2023 16:02

cameel force-pushed the newAnalysis branch from 8c1508c to 98008bd Compare October 30, 2023 16:02

cameel force-pushed the new-analysis-remove-polymorphic-instance branch 2 times, most recently from acef62d to 8c31a4f Compare October 30, 2023 18:39

cameel mentioned this pull request Oct 31, 2023

Distinguish fixed free types from generic type variables in experimental solidity. #14570

Closed

cameel force-pushed the newAnalysis branch from 3078a9f to 010be71 Compare November 1, 2023 10:26

cameel force-pushed the new-analysis-remove-polymorphic-instance branch from 8c31a4f to 5db6e6c Compare November 1, 2023 11:28

cameel added 2 commits November 1, 2023 12:29

Tests covering the uses of polymorphic functions and types

9530bec

Function types in typeClassFunctions() do not need `polymorphicInst…

f06aed8

…ance()`

cameel added 2 commits November 1, 2023 12:29

Type classes currently cannot take type parameters

c078827

Remove TypeInference::unifyGeneralized()

0162b8e

cameel force-pushed the new-analysis-remove-polymorphic-instance branch from 5db6e6c to 0162b8e Compare November 1, 2023 11:30

ekpyron reviewed Nov 1, 2023

View reviewed changes

ekpyron approved these changes Nov 1, 2023

View reviewed changes

cameel merged commit 5f43e35 into newAnalysis Nov 1, 2023
1 check passed

cameel deleted the new-analysis-remove-polymorphic-instance branch November 1, 2023 19:13

cameel added a commit that referenced this pull request Dec 8, 2023

Merge pull request #14635 from ethereum/new-analysis-remove-polymorph…

826c168

…ic-instance Adjust `polymorphicInstance()`

cameel added a commit that referenced this pull request Dec 13, 2023

Merge pull request #14635 from ethereum/new-analysis-remove-polymorph…

561bddc

…ic-instance Adjust `polymorphicInstance()`

cameel added a commit that referenced this pull request Dec 13, 2023

Merge pull request #14635 from ethereum/new-analysis-remove-polymorph…

0b2604d

…ic-instance Adjust `polymorphicInstance()`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust `polymorphicInstance()` #14635

Adjust `polymorphicInstance()` #14635

cameel commented Oct 23, 2023 •

edited

Loading

ekpyron commented Oct 23, 2023

cameel commented Oct 23, 2023

ekpyron commented Oct 23, 2023 •

edited

Loading

ekpyron commented Oct 23, 2023 •

edited

Loading

cameel commented Oct 27, 2023

cameel commented Oct 30, 2023

ekpyron commented Oct 30, 2023

cameel commented Nov 1, 2023

ekpyron commented Nov 1, 2023

ekpyron Nov 1, 2023

ekpyron Nov 1, 2023

cameel Nov 1, 2023

Adjust polymorphicInstance() #14635

Adjust polymorphicInstance() #14635

Conversation

cameel commented Oct 23, 2023 • edited Loading

ekpyron commented Oct 23, 2023

cameel commented Oct 23, 2023

ekpyron commented Oct 23, 2023 • edited Loading

ekpyron commented Oct 23, 2023 • edited Loading

cameel commented Oct 27, 2023

cameel commented Oct 30, 2023

ekpyron commented Oct 30, 2023

cameel commented Nov 1, 2023

ekpyron commented Nov 1, 2023

ekpyron Nov 1, 2023

Choose a reason for hiding this comment

ekpyron Nov 1, 2023

Choose a reason for hiding this comment

cameel Nov 1, 2023

Choose a reason for hiding this comment

Adjust `polymorphicInstance()` #14635

Adjust `polymorphicInstance()` #14635

cameel commented Oct 23, 2023 •

edited

Loading

ekpyron commented Oct 23, 2023 •

edited

Loading

ekpyron commented Oct 23, 2023 •

edited

Loading