From cf19c70b86f630184c2163b81b57cdbed3f3864e Mon Sep 17 00:00:00 2001
From: George Cook <george@tantawowa.com>
Date: Tue, 4 May 2021 04:47:52 +0200
Subject: [PATCH] adds warning for mismatched class member accessibility (#402)

* adds warning for mismatched class member accessibility

* updates overridden accessType message, as requested in pr review
---
 src/DiagnosticMessages.ts        |  5 +++
 src/files/BrsFile.Class.spec.ts  | 53 ++++++++++++++++++++++++++++++++
 src/validators/ClassValidator.ts | 20 ++++++++++++
 3 files changed, 78 insertions(+)

diff --git a/src/DiagnosticMessages.ts b/src/DiagnosticMessages.ts
index b9f5703e9..542b624b7 100644
--- a/src/DiagnosticMessages.ts
+++ b/src/DiagnosticMessages.ts
@@ -625,6 +625,11 @@ export let DiagnosticMessages = {
         message: `Missing expression(s) in 'dim' statement`,
         code: 1121,
         severity: DiagnosticSeverity.Error
+    }),
+    mismatchedOverriddenMemberVisibility: (childClassName: string, memberName: string, childAccessModifier: string, ancestorAccessModifier: string, ancestorClassName: string) => ({
+        message: `Access modifier mismatch: '${memberName}' is ${childAccessModifier} in type '${childClassName}' but is ${ancestorAccessModifier} in base type '${ancestorClassName}'.`,
+        code: 1122,
+        severity: DiagnosticSeverity.Error
     })
 };
 
diff --git a/src/files/BrsFile.Class.spec.ts b/src/files/BrsFile.Class.spec.ts
index fe25e42ec..f5ed1f527 100644
--- a/src/files/BrsFile.Class.spec.ts
+++ b/src/files/BrsFile.Class.spec.ts
@@ -830,6 +830,59 @@ describe('BrsFile BrighterScript classes', () => {
         });
     });
 
+    it('detects overridden methods with different visibility', () => {
+        program.addOrReplaceFile({ src: `${rootDir}/source/main.bs`, dest: 'source/main.bs' }, `
+            class Animal
+                sub speakInPublic()
+                end sub
+                protected sub speakWithFriends()
+                end sub
+                private sub speakWithFamily()
+                end sub
+            end class
+            class Duck extends Animal
+                private override sub speakInPublic()
+                end sub
+                public override sub speakWithFriends()
+                end sub
+                override sub speakWithFamily()
+                end sub
+            end class
+        `);
+        program.validate();
+        expect(program.getDiagnostics()[0]).to.exist.and.to.include({
+            ...DiagnosticMessages.mismatchedOverriddenMemberVisibility('Duck', 'speakInPublic', 'private', 'public', 'Animal')
+        });
+        expect(program.getDiagnostics()[1]).to.exist.and.to.include({
+            ...DiagnosticMessages.mismatchedOverriddenMemberVisibility('Duck', 'speakWithFriends', 'public', 'protected', 'Animal')
+        });
+        expect(program.getDiagnostics()[2]).to.exist.and.to.include({
+            ...DiagnosticMessages.mismatchedOverriddenMemberVisibility('Duck', 'speakWithFamily', 'public', 'private', 'Animal')
+        });
+    });
+    it('allows overridden methods with matching visibility', () => {
+        program.addOrReplaceFile({ src: `${rootDir}/source/main.bs`, dest: 'source/main.bs' }, `
+            class Animal
+                sub speakInPublic()
+                end sub
+                protected sub speakWithFriends()
+                end sub
+                private sub speakWithFamily()
+                end sub
+            end class
+            class Duck extends Animal
+                override sub speakInPublic()
+                end sub
+                protected override sub speakWithFriends()
+                end sub
+                private override sub speakWithFamily()
+                end sub
+            end class
+        `);
+        program.validate();
+        expect(program.getDiagnostics()).to.be.empty;
+    });
+
     it('detects extending unknown parent class', () => {
         program.addOrReplaceFile('source/main.brs', `
             class Duck extends Animal
diff --git a/src/validators/ClassValidator.ts b/src/validators/ClassValidator.ts
index f2955c6e3..252adece5 100644
--- a/src/validators/ClassValidator.ts
+++ b/src/validators/ClassValidator.ts
@@ -10,6 +10,7 @@ import { isCallExpression, isClassFieldStatement, isClassMethodStatement, isCust
 import type { BscFile, BsDiagnostic } from '../interfaces';
 import { createVisitor, WalkMode } from '../astUtils';
 import type { BrsFile } from '../files/BrsFile';
+import { TokenKind } from '../lexer';
 
 export class BsClassValidator {
     private scope: Scope;
@@ -223,6 +224,25 @@ export class BsClassValidator {
                                 range: member.range
                             });
                         }
+
+                        //child member has different visiblity
+                        if (
+                            //is a method
+                            isClassMethodStatement(member) &&
+                            (member.accessModifier?.kind ?? TokenKind.Public) !== (ancestorAndMember.member.accessModifier?.kind ?? TokenKind.Public)
+                        ) {
+                            this.diagnostics.push({
+                                ...DiagnosticMessages.mismatchedOverriddenMemberVisibility(
+                                    classStatement.name.text,
+                                    ancestorAndMember.member.name?.text,
+                                    member.accessModifier?.text || 'public',
+                                    ancestorAndMember.member.accessModifier?.text || 'public',
+                                    ancestorAndMember.classStatement.getName(ParseMode.BrighterScript)
+                                ),
+                                file: classStatement.file,
+                                range: member.range
+                            });
+                        }
                     }
 
                     if (isClassMethodStatement(member)) {