Give Diagnostic messages on improper usage of Generic

src/Analysis/Ast/Impl/Analyzer/Evaluation/ExpressionEval.Generics.cs

@@ -16,12 +16,15 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using Microsoft.Python.Analysis.Diagnostics;
 using Microsoft.Python.Analysis.Specializations.Typing;
 using Microsoft.Python.Analysis.Specializations.Typing.Types;
 using Microsoft.Python.Analysis.Types;
 using Microsoft.Python.Analysis.Values;
 using Microsoft.Python.Core;
+using Microsoft.Python.Parsing;
 using Microsoft.Python.Parsing.Ast;
+using ErrorCodes = Microsoft.Python.Analysis.Diagnostics.ErrorCodes;
 
 namespace Microsoft.Python.Analysis.Analyzer.Evaluation {
     internal sealed partial class ExpressionEval {
@@ -59,40 +62,58 @@ private IMember GetValueFromGeneric(IMember target, Expression expr) {
         }
 
         /// <summary>
-        /// Given generic type and list of indices in the expression like
-        /// Generic[T1, T2, ...] or List[str] creates generic class base
-        /// (if the former) on specific type (if the latter).
+        /// Returns whether the arguments to Generic are valid
         /// </summary>
-        private IMember CreateSpecificTypeFromIndex(IGenericType gt, IReadOnlyList<IMember> indices, Expression expr) {
-            // See which ones are generic parameters as defined by TypeVar() 
-            // and which are specific types. Normally there should not be a mix.
-            var genericTypeArgs = indices.OfType<IGenericTypeDefinition>().ToArray();
-            var specificTypes = indices.Where(i => !(i is IGenericTypeDefinition)).OfType<IPythonType>().ToArray();
-
-            if (genericTypeArgs.Length > 0 && genericTypeArgs.Length != indices.Count) {
-                // TODO: report that some type arguments are not declared with TypeVar.
+        private bool GenericClassParameterValid(IReadOnlyList<IGenericTypeDefinition> genericTypeArgs, IReadOnlyList<IMember> args, Expression expr) {
+            // All arguments to Generic must be type parameters
+            // e.g. Generic[T, str] throws a runtime error
+            if (genericTypeArgs.Count != args.Count) {
+                ReportDiagnostics(Module.Uri, new DiagnosticsEntry(
+                    Resources.GenericNotAllTypeParameters,
+                    GetLocation(expr).Span,
+                    ErrorCodes.TypingGenericArguments,
+                    Severity.Error,
+                    DiagnosticSource.Analysis));
+                return false;
             }
-            if (specificTypes.Length > 0 && specificTypes.Length != indices.Count) {
-                // TODO: report that arguments are not specific types or are not declared.
+
+            // All arguments to Generic must be distinct
+            if (genericTypeArgs.Distinct().Count() != genericTypeArgs.Count) {
+                ReportDiagnostics(Module.Uri, new DiagnosticsEntry(
+                   Resources.GenericNotAllUnique,
+                   GetLocation(expr).Span,
+                   ErrorCodes.TypingGenericArguments,
+                   Severity.Error,
+                   DiagnosticSource.Analysis));
+                return false;
             }
 
+            return true;
+        }
+
+        /// <summary>
+        /// Given generic type and list of arguments in the expression like
+        /// Generic[T1, T2, ...] or List[str] creates generic class base
+        /// (if the former) on specific type (if the latter).
+        /// </summary>
+        private IMember CreateSpecificTypeFromIndex(IGenericType gt, IReadOnlyList<IMember> args, Expression expr) {
+            var genericTypeArgs = args.OfType<IGenericTypeDefinition>().ToArray();
+
             if (gt.Name.EqualsOrdinal("Generic")) {
-                // Generic[T1, T2, ...] expression. Create generic base for the class.
-                if (genericTypeArgs.Length > 0) {
-                    return new GenericClassParameter(genericTypeArgs, Module);
-                } else {
-                    // TODO: report too few type arguments for Generic[].
+                if (!GenericClassParameterValid(genericTypeArgs, args, expr)) {
                     return UnknownType;
                 }
+
+                // Generic[T1, T2, ...] expression. Create generic base for the class.
+                return new GenericClassParameter(genericTypeArgs, Module);
             }
 
             // For other types just use supplied arguments
-            if (indices.Count > 0) {
-                return gt.CreateSpecificType(new ArgumentSet(indices, expr, this));
+            if (args.Count > 0) {
+                return gt.CreateSpecificType(new ArgumentSet(args, expr, this));
             }
-            // TODO: report too few type arguments for the generic expression.
-            return UnknownType;
 
+            return UnknownType;
         }
 
         private IReadOnlyList<IMember> EvaluateIndex(IndexExpression expr) {
@@ -194,7 +215,7 @@ private static IReadOnlyList<IPythonType> GetSpecificTypeFromArgumentValue(objec
                     var itemType = iter.GetIterator().Next.GetPythonType();
                     if (!itemType.IsUnknown()) {
                         specificTypes.Add(itemType);
-                    } else if(argumentValue is IPythonInstance inst) {
+                    } else if (argumentValue is IPythonInstance inst) {
                         specificTypes.Add(inst.GetPythonType());
                     }
                     break;

src/Analysis/Ast/Impl/Diagnostics/ErrorCodes.cs

@@ -26,5 +26,6 @@ public static class ErrorCodes {
         public const string VariableNotDefinedGlobally= "variable-not-defined-globally";
         public const string VariableNotDefinedNonLocal = "variable-not-defined-nonlocal";
         public const string ReturnInInit = "return-in-init";
+        public const string TypingGenericArguments = "typing-generic-arguments";
     }
 }

src/Analysis/Ast/Impl/Resources.resx

@@ -192,4 +192,10 @@
   <data name="ReturnInInit" xml:space="preserve">
     <value>Explicit return in __init__ </value>
   </data>
+  <data name="GenericNotAllTypeParameters" xml:space="preserve">
+    <value>Arguments to Generic must all be type parameters.</value>
+  </data>
+  <data name="GenericNotAllUnique" xml:space="preserve">
+    <value>Arguments to Generic must all be unique.</value>
+  </data>
 </root>

src/Analysis/Ast/Test/LintGenericTests.cs

@@ -0,0 +1,86 @@
+using System.Linq;
+using System.Threading.Tasks;
+using FluentAssertions;
+using Microsoft.Python.Analysis.Tests.FluentAssertions;
+using Microsoft.Python.Parsing.Tests;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using TestUtilities;
+
+
+namespace Microsoft.Python.Analysis.Tests {
+
+    [TestClass]
+    public class LintGenericTests : AnalysisTestBase {
+
+        public const string GenericSetup = @"
+from typing import Generic, TypeVar
+T = TypeVar('T', int, str)
+T1 = TypeVar('T1', int, str)
+
+_X = TypeVar('_X', str, int)
+_T = _X
+";
+
+        public TestContext TestContext { get; set; }
+
+        [TestInitialize]
+        public void TestInitialize()
+            => TestEnvironmentImpl.TestInitialize($"{TestContext.FullyQualifiedTestClassName}.{TestContext.TestName}");
+
+        [TestCleanup]
+        public void Cleanup() => TestEnvironmentImpl.TestCleanup();
+
+        [DataRow("x = Generic[T, str]")]
+        [DataRow("x = Generic[T, T1, int]")]
+        [DataRow("x = Generic[T, str, int, T1]")]
+        [DataRow("x = Generic[str, int]")]
+        [DataRow("x = Generic[str]")]
+        [DataTestMethod, Priority(0)]
+        public async Task GenericNotAllTypeParameters(string decl) {
+            string code = GenericSetup + decl;
+
+            var analysis = await GetAnalysisAsync(code);
+            analysis.Diagnostics.Should().HaveCount(1);
+
+            var diagnostic = analysis.Diagnostics.ElementAt(0);
+            var start = decl.IndexOf("Generic") + 1;
+            // adding 1 because SourceSpan.End is exclusive and another 1 because SourceSpan is 1-indexed
+            var end = decl.IndexOf("]", start) + 2;
+
+            diagnostic.SourceSpan.Should().Be(8, start, 8, end);
+            diagnostic.ErrorCode.Should().Be(Diagnostics.ErrorCodes.TypingGenericArguments);
+            diagnostic.Message.Should().Be(Resources.GenericNotAllTypeParameters);
+        }
+
+        [DataRow("x = Generic[_T, _X]")]
+        [DataRow("x = Generic[_T, T, T1, _X]")]
+        [DataRow("x = Generic[_T,_T, T]")]
+        [DataRow("x = Generic[T,T]")]
+        [DataTestMethod, Priority(0)]
+        public async Task GenericDuplicateArguments(string decl) {
+            string code = GenericSetup + decl;
+            var analysis = await GetAnalysisAsync(code);
+            analysis.Diagnostics.Should().HaveCount(1);
+
+            var diagnostic = analysis.Diagnostics.ElementAt(0);
+            var start = decl.IndexOf("Generic") + 1;
+            // adding 1 because SourceSpan.End is exclusive and another 1 because SourceSpan is 1-indexed
+            var end = decl.IndexOf("]", start) + 2;
+            diagnostic.SourceSpan.Should().Be(8, start, 8, end);
+
+            diagnostic.ErrorCode.Should().Be(Diagnostics.ErrorCodes.TypingGenericArguments);
+            diagnostic.Message.Should().Be(Resources.GenericNotAllUnique);
+        }
+
+        [DataRow("x = Generic[_X, T]")]
+        [DataRow("x = Generic[T1, T]")]
+        [DataRow("x = Generic[T]")]
+        [DataRow("x = Generic[T,T1, _X]")]
+        [DataTestMethod, Priority(0)]
+        public async Task GenericArgumentsNoDiagnosticOnValid(string decl) {
+            string code = GenericSetup + decl;
+            var analysis = await GetAnalysisAsync(code);
+            analysis.Diagnostics.Should().HaveCount(0);
+        }
+    }
+}