UInt(width=...) constructor broken instead of deprecated?

[edwardcwang]

According to https://github.com/freechipsproject/chisel3/blob/6e12ed9fd7a771eb30f44b8e1c4ab33f6ad8e0a6/src/main/scala/chisel3/package.scala#L142 the UInt(width=...) methods should only be deprecated (as opposed to removed). However, the following block seems to cause a type mismatch error with the latest 3.0 snapshot:

class TestBlock extends Module {
	val io = IO(new Bundle {
		val output = Output(UInt(width=3))
	})
}

type mismatch;
[error]  found   : Int(3)
[error]  required: chisel3.internal.firrtl.Width
[error] 		val output = Output(UInt(width=3))

It seems that this hard breaks some previously-valid Chisel3 code (as opposed to giving a warning, which seems like a regression).

sbt 0.13.12, scala 2.11.8

[ducky64]

The line you're referring to is concerned with the UInt.width(width=...) method.

We got rid of the UInt(...) method because a bare int in the argument could be ambiguously interpreted as a UInt literal, which we don't want to have happen.

[edwardcwang]

Is this intended to be a breaking change? I was under the impression that the legacy constructors would be made deprecated and not removed entirely. This was working Chisel3 code last semester and now has stopped compiling...

[ducky64]

I don't know. Everything that's on a deprecation schedule is meant to disappear eventually, so the amount of cruft we have to shim doesn't just keep growing. The change was made here:

https://github.com/freechipsproject/chisel3/pull/372/files

@deprecated("use UInt(width.W)", "chisel3, will be removed by end of 2016")
def apply(dir: Option[Direction] = None, width: Int): UInt = apply(width.W)

and as it's now past 2016, I guess it's gone. You can do more sleuthing to find the exact PR where that line was removed for context.

You can argue when exactly it should be removed, but I will say that UInt(value: Int) and UInt(width=width:Int) is a really terrible and confusing API.

[edwardcwang]

I agree it's not the most ideal API; yet at the time it's nice to not break working code...

[ducky64]

I assume that since the original PR left it in, it was removed in a subsequent, probably reviewed PR. Feel free to argue for its inclusion again, but you should find the context in which it was removed (I don't remember the details), and if it was indeed removed because the deprecation notice said "end of 2016", then you also need to consider that we can't support old, unintuitive, tricky, deprecated APIs forever.

[edwardcwang]

There doesn't seem to be a resolution to this...?

[edwardcwang]

One solution could be to error out with a descriptive error message ("Using removed API, please use alternative API X instead"), but I don't know how to do that at compile time (vs run-time).

[ducky64]

Since you didn't want to do the sleuthing, I did it for you. Turns out it was removed in #617, looks like as part of general cleanup (also, it silently drops the direction argument, which is bad).

I think it's a pretty terrible and confusing API and I don't particularly want to bring it back. It was removed mid-2017, so we didn't violate any guarantees (the deprecation message said "will be removed by the end of 2016", which happened) and people have had plenty of time to migrate. Chisel3's package.scala is also supposed to be on a rolling deprecation schedule, as opposed to compatibility.scala which will maintain old APIs for longer.

@edwardcwang Also, as I've heard no complaints about this, so practically this isn't a problem from the (sizable number of) people using chisel3. Please don't try to resurrect terribad dead APIs for the sake doing so. If this is part of a nontrivial use case that absolutely does not / can not want to migrate to the new, non-awful style, then please link to it and we can further discuss in that context.

As far as I know, there isn't an @error annotation, though it would be possible to make one using the macro system. But in the absence of use cases, I don't think that's worth the effort.

[edwardcwang]

Thanks for looking into this.

I agree and am not suggesting that we should bring back the terribad API for actual usage; the problem is that from the perspective of Chisel users, it feels like Chisel broke instead of their code broke. It would be nice to make it clear to users that "you were using removed features; please fix your code" instead of some compile error (huh? firrtl? "chisel people must have broken stuff again, #@!$")

I do like the idea of the @error annotation; due to the plethora of Chisel materials online, sometimes users accidentally try to use things in the Chisel2 way, and perhaps the @error notation may be a more user-friendly way than "@#!$ing chisel broke my working code".

[ducky64]

Sure, I guess I can write up a @removedApi annotation.

[ducky64]

Bad news, the API is so terrible it's incompatible with macros.

macro applications do not support named and/or default arguments

This uses both, I don't see a good way of causing a compile time error on method invocation.
The other option is to have it throw a runtime error (potentially with compile time deprecation), though that leaves the possibility of having code lurking code waiting for the perfect chance to throw a random runtime error. So I don't really like that option.

The @removedApi annotation may have future value when we start getting rid of the APIs that we say we will remove by the end of 2017, but that's another problem for another day.

[edwardcwang]

If any function invokes a function which is annotated with @removedapi, it should also fail at compile-time, correct?

Then maybe we could do something like

@removedapi
def otherSaneRemovedAPI(x: Int): Unit = {
 // stuff
}

@removedapi
def removed(): Unit = {
  // Write something useful here
}

def UInt(value: Int = 999, width: Int = 123) = {
 removed()
}

// In user code
def user_code() = {
  otherSaneRemovedAPI(123) // <-- compile-time error
  UInt(9, width=32) // <-- also compile-time error since the UInt call will generate a compile-time error?
}

[ducky64]

It's easy to make things fail at compile time, but not easy to make them fail informatively. The example above would fail, but where it would fail is that def UInt would fail to compile. If we shipped that in Chisel, Chisel would fail to compile, regardless of what the user code does.

Basically, the original idea was to use a def macro, which transforms the invocation site of a function, and have that transform error out with an informative message. Again, the problem is that it doesn't support named / default arguments. Since they key is erroring at the use site (like source locator macros, which need to be determined at the use site), a level of indirection at runtime (within a function call) doesn't help.

I think. Unless I'm missing something obvious...

[ducky64]

Resolution: perhaps add it to the wiki

[ducky64]

@ucbjrl