Deprecate auto-application of empty argument lists to parameterless functions #2124

jared-barocsi · 2021-09-16T00:44:14Z

Scala 3 now enforces a distinction between nullary parameter-less functions and nullary functions with empty argument lists. Previously, Scala 2 would auto-apply an empty parameter list onto any invocation of a nullary parameter-less function.

As such, in order to migrate to Scala 3, any nullary function that doesn't induce a side effect, e.g. asUInt, must not include a parameter list in its invocation. To deprecate the use of empty parameter lists for these functions, dummy versions of these functions are included which explicitly allow use of an empty parameter list (for example, a default dummy argument or a vararg).

Contributor Checklist

Did you add Scaladoc to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous printlns/debugging code?
Did you specify the type of improvement?
Did you add appropriate documentation in docs/src?
Did you state the API impact?
Did you specify the code generation impact?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

code refactoring

API Impact

No effect

Backend Code Generation Impact

No impact

Desired Merge Strategy

Squash and merge

Release Notes

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels?
Did you mark the proper milestone (Bug fix: 3.3.x, [small] API extension: 3.4.x, API modification or big change: 3.5.0)?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you mark as Please Merge?

core/src/main/scala/chisel3/Bits.scala

core/src/main/scala/chisel3/When.scala

core/src/main/scala/chisel3/internal/Builder.scala

src/main/scala/chisel3/util/Decoupled.scala

davidbiancolin · 2021-09-17T06:03:35Z

Inb4 "asUInt does have a side effect".

aswaterman · 2021-09-17T06:16:24Z

Inb4 "asUInt does have a side effect".

trigger warning

core/src/main/scala/chisel3/Bits.scala

jackkoenig · 2021-09-17T19:35:37Z

Inb4 "asUInt does have a side effect".

Go away.

In all seriousness though for anyone wondering though--while basically any Chisel API does have side effects, we go to great lengths to hide those side effects from the user such that Chisel APIs appear to be side effect free and therefore will follow the Scala style of not having parentheses.

jackkoenig

Please follow Megan's suggestion on the deprecation messages (ie. they need to tell the user what to do). As I said in the comment, I don't think they need to mention the name of the method, but do need to say something like "use form without parentheses" or "use form of method without parentheses" or something.

Please also make the dummy argument style consistent. Since varargs are the only style that work for macros, I'd apply that throughout.

Also, please leave a blank line between a method definition and the start of the following definition (including when it's a deprecation annotation).

core/src/main/scala/chisel3/Bits.scala

…hisel3 into migrate-zero-arity-funcs

…e-zero-arity-funcs

jackkoenig

Every deprecation warning needs the version to be "Chisel 3.5", not just "3.5" because the Scala compiler either doesn't know or at least doesn't report the name of the project that deprecation warnings come from.

jackkoenig · 2021-10-04T17:35:56Z

core/src/main/scala/chisel3/Bits.scala

+  final def asSInt: SInt = macro SourceInfoTransform.noArg
+  @deprecated("Calling this function with an empty argument list is invalid in Scala 3. Use the form without parentheses instead", "3.5")


Suggested change

final def asSInt: SInt = macro SourceInfoTransform.noArg

@deprecated("Calling this function with an empty argument list is invalid in Scala 3. Use the form without parentheses instead", "3.5")

final def asSInt: SInt = macro SourceInfoTransform.noArg

@deprecated("Calling this function with an empty argument list is invalid in Scala 3. Use the form without parentheses instead", "3.5")

jackkoenig · 2021-10-04T17:36:09Z

core/src/main/scala/chisel3/Bits.scala

+  final def asBools: Seq[Bool] = macro SourceInfoTransform.noArg
+  @deprecated("Calling this function with an empty argument list is invalid in Scala 3. Use the form without parentheses instead", "3.5")


Suggested change

final def asBools: Seq[Bool] = macro SourceInfoTransform.noArg

@deprecated("Calling this function with an empty argument list is invalid in Scala 3. Use the form without parentheses instead", "3.5")

final def asBools: Seq[Bool] = macro SourceInfoTransform.noArg

@deprecated("Calling this function with an empty argument list is invalid in Scala 3. Use the form without parentheses instead", "3.5")

core/src/main/scala/chisel3/Bits.scala

src/main/scala/chisel3/util/Decoupled.scala

sequencer · 2021-10-07T09:40:53Z

this breaks dsptools.

[#4] [error] /Users/sequencer/projects/playground/wtf/dependencies/dsptools/src/main/scala/dsptools/numbers/chisel_types/FixedPointTypeClass.scala:40:43: ambiguous reference to overloaded definition,
[#4] [error] both method unary_- in class FixedPoint of type (implicit sourceInfo: chisel3.internal.sourceinfo.SourceInfo, implicit compileOptions: chisel3.CompileOptions)chisel3.experimental.FixedPoint
[#4] [error] and  macro method unary_- in class FixedPoint of type => chisel3.experimental.FixedPoint
[#4] [error] match expected type chisel3.experimental.FixedPoint
[#4] [error]   def negate(f: FixedPoint): FixedPoint = -f
[#4] [error]                                           ^
[#4] [error] /Users/sequencer/projects/playground/wtf/dependencies/dsptools/src/main/scala/dsptools/numbers/chisel_types/IntervalTypeClass.scala:44:39: ambiguous reference to overloaded definition,
[#4] [error] both method unary_- in class Interval of type (implicit sourceInfo: chisel3.internal.sourceinfo.SourceInfo, implicit compileOptions: chisel3.CompileOptions)chisel3.experimental.Interval
[#4] [error] and  macro method unary_- in class Interval of type => chisel3.experimental.Interval
[#4] [error] match expected type chisel3.experimental.Interval
[#4] [error]   def negate(f: Interval): Interval = -(f)
[#4] [error]                                       ^
[#4] [error] two errors found

…erless functions (#2124)" This reverts commit 110705e.

sequencer · 2021-10-07T13:51:18Z

reproducible in https://github.com/sequencer/playground/runs/3827353710

chick · 2021-10-07T19:22:32Z

@jackkoenig @sequencer I think I might have figured this out. Looks like the macro expansions of do_unary_- are not defined for FixedPoint and Interval, not sure how everything worked before here but I'm testing a fix now

In `Bits.scala`, `FixedPoint` and `Interval` did not defeine the `do_unary_-` methods (the `do_`) was missing The recent PR #2124 combined with the above fact made DspTools break. This fix is necessary to get that repo to build.

sequencer · 2021-10-07T23:24:19Z

super fast!

Ref. chipsalliance/chisel#2124

Due to inlining of Boolean expressions, the following circuit is handled directly by the VerilogEmitter: input a: UInt<4> input b: SInt<1> output o: UInt<5> o <= dshl(a, asUInt(cvt(b))) Priot to this change, this could crash due to mishandling of cvt in the logic to inject parentheses based on Verilog precedence rules. This is a corner case, but similar bugs would drop up if we open up the VerilogEmitter to more expression inlining.

Migrate nullary funcs to parameterless versions

ef4f8a0

jared-barocsi force-pushed the migrate-zero-arity-funcs branch from 3ffbca4 to ef4f8a0 Compare September 16, 2021 00:46

Merge branch 'master' into migrate-zero-arity-funcs

23aaecb

jared-barocsi marked this pull request as ready for review September 16, 2021 20:02

mwachs5 reviewed Sep 17, 2021

View reviewed changes

core/src/main/scala/chisel3/Bits.scala Outdated Show resolved Hide resolved

mwachs5 reviewed Sep 17, 2021

View reviewed changes

core/src/main/scala/chisel3/When.scala Outdated Show resolved Hide resolved

mwachs5 reviewed Sep 17, 2021

View reviewed changes

core/src/main/scala/chisel3/internal/Builder.scala Show resolved Hide resolved

mwachs5 reviewed Sep 17, 2021

View reviewed changes

core/src/main/scala/chisel3/internal/Builder.scala Show resolved Hide resolved

mwachs5 reviewed Sep 17, 2021

View reviewed changes

src/main/scala/chisel3/util/Decoupled.scala Show resolved Hide resolved

jackkoenig reviewed Sep 17, 2021

View reviewed changes

core/src/main/scala/chisel3/Bits.scala Outdated Show resolved Hide resolved

jackkoenig requested changes Sep 17, 2021

View reviewed changes

jackkoenig reviewed Sep 17, 2021

View reviewed changes

core/src/main/scala/chisel3/Bits.scala Outdated Show resolved Hide resolved

jared-barocsi added 2 commits September 17, 2021 16:31

Make deprecation message and dummy arguments clear and consistent

c75cd6b

Merge branch 'migrate-zero-arity-funcs' of github.com:jared-barocsi/c…

26fa2f4

…hisel3 into migrate-zero-arity-funcs

ekiwi mentioned this pull request Sep 20, 2021

remove parenthesis from asBool, asUInt methods #2088

Closed

jared-barocsi added 2 commits September 29, 2021 16:43

Merge branch 'master' of github.com:chipsalliance/chisel3 into migrat…

c1c37e9

…e-zero-arity-funcs

Fix ambiguity of SInt unary negation operator

b7a0d6c

jackkoenig requested changes Oct 4, 2021

View reviewed changes

mwachs5 mentioned this pull request Oct 4, 2021

Deprecate toBools in Bits (should be asBools) #2143

Closed

jared-barocsi and others added 2 commits October 4, 2021 13:02

Change deprecation versions to Chisel 3.5

97a2384

Merge branch 'master' into migrate-zero-arity-funcs

c7891dc

jackkoenig added this to the 3.5.0 milestone Oct 5, 2021

jackkoenig approved these changes Oct 5, 2021

View reviewed changes

jackkoenig enabled auto-merge (squash) October 5, 2021 18:52

jackkoenig added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Oct 5, 2021

Merge branch 'master' into migrate-zero-arity-funcs

501803e

jackkoenig merged commit 110705e into chipsalliance:master Oct 5, 2021

sequencer added a commit that referenced this pull request Oct 7, 2021

Revert "Deprecate auto-application of empty argument lists to paramet…

3952ced

…erless functions (#2124)" This reverts commit 110705e.

sequencer mentioned this pull request Oct 7, 2021

Revert "Deprecate auto-application of empty argument lists to parameterless functions (#2124)" #2152

Closed

14 tasks

chick mentioned this pull request Oct 7, 2021

fixed bug with unary minus on FixedPoint and Interval #2154

Merged

13 tasks

This was referenced Oct 8, 2021

Add nullary .fire to Valid and deprecate dummy version #2156

Merged

Ergonomics of asUInt #1398

Closed

ekiwi mentioned this pull request Oct 20, 2021

Test output when using Verilator backend could be less verbose ucb-bar/chiseltest#412

Closed

carlosedp added a commit to carlosedp/chiselv that referenced this pull request Oct 20, 2021

Cleanup empty argument functions (litvalue, asSint, asUint)

c5f4964

Ref. chipsalliance/chisel#2124

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate auto-application of empty argument lists to parameterless functions #2124

Deprecate auto-application of empty argument lists to parameterless functions #2124

jared-barocsi commented Sep 16, 2021

davidbiancolin commented Sep 17, 2021

aswaterman commented Sep 17, 2021

jackkoenig commented Sep 17, 2021

jackkoenig left a comment

jackkoenig left a comment

jackkoenig Oct 4, 2021

jackkoenig Oct 4, 2021

sequencer commented Oct 7, 2021

sequencer commented Oct 7, 2021

chick commented Oct 7, 2021

sequencer commented Oct 7, 2021

		final def asSInt: SInt = macro SourceInfoTransform.noArg
		@deprecated("Calling this function with an empty argument list is invalid in Scala 3. Use the form without parentheses instead", "3.5")

		final def asBools: Seq[Bool] = macro SourceInfoTransform.noArg
		@deprecated("Calling this function with an empty argument list is invalid in Scala 3. Use the form without parentheses instead", "3.5")

Deprecate auto-application of empty argument lists to parameterless functions #2124

Deprecate auto-application of empty argument lists to parameterless functions #2124

Conversation

jared-barocsi commented Sep 16, 2021

Contributor Checklist

Type of Improvement

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

davidbiancolin commented Sep 17, 2021

aswaterman commented Sep 17, 2021

jackkoenig commented Sep 17, 2021

jackkoenig left a comment

Choose a reason for hiding this comment

jackkoenig left a comment

Choose a reason for hiding this comment

jackkoenig Oct 4, 2021

Choose a reason for hiding this comment

jackkoenig Oct 4, 2021

Choose a reason for hiding this comment

sequencer commented Oct 7, 2021

sequencer commented Oct 7, 2021

chick commented Oct 7, 2021

sequencer commented Oct 7, 2021